Repository navigation
apps.cozystack.io aggregated API ignores dryRun: kubectl --dry-run=server persists writes #3982
Description
Activity
Hi lf (@lfinmauritius)! I'm Dosu and I'm helping the Cozystack team.
Your analysis is correct and matches two earlier reports of the same bug: #2981 (first reported against the
Updatepath on v1.5.0) and #3971 (confirming the behavior across the aggregated apiserver). There is already an open fix in PR #3937 that addresses the root cause acrossCreate,Update, andDelete.The PR description explains that the problem goes deeper than simply not checking
dryrun.IsDryRun(): passing the incomingmetav1.*Optionsthroughclient.*Options{Raw: options}is also insufficient because controller-runtime'sAs*Options()methods rebuildRawfrom the typed struct fields, silently droppingDryRun,FieldManager,FieldValidation, delete propagation, grace periods, and preconditions [1].On
maintoday, the specific code paths are:- Create passes
&client.CreateOptions{Raw: options}[2] — looks correct but still loses options through controller-runtime's rebuilding. - Update (retry path) constructs a fresh empty
&metav1.UpdateOptions{}, discarding the caller's options entirely [3]. - Update (force-create/upsert path) calls
r.Create(ctx, obj, createValidation, &metav1.CreateOptions{})with no options at all [4]. - Delete passes
&client.DeleteOptions{Raw: options}[5] — same controller-runtime rebuilding issue as Create.
PR #3937 replaces all of these with new typed-option converters (
ClientCreateOptions,CreateOptionsFromUpdate,ClientUpdateOptions,ClientDeleteOptions) in a newpkg/registry/write_options.go, and also fixes the same class of bug inpkg/registry/core/tenantsecret/rest.goandpkg/registry/sdn/securitygroup/rest.go[1]. This covers theDeletecase you flagged as most concerning — a--dry-run=serverdelete would indeed have removed the backing HelmRelease for real under the current code.As of now, PR #3937 is still open and not yet merged, so both
mainand v1.6.1 remain affected [6].To reply, just mention @dosu.
Share context across your team and agents. Try Dosu.
- Create passes
Hi! I'll take a look on this soon.
One aggregated storage sits outside the fix in #3937 and still writes on a server dry-run:
taps.core.cozystack.io.pkg/registry/core/tap/rest.gotakes the options and discards them —Createat line 182 andDeleteat 276 both bind the incoming*metav1.CreateOptions/*metav1.DeleteOptionsto_and construct fresh ones for their dynamic client. So there is nothing to nil out, which is how it escaped the controller-runtime mechanism this issue is about: the other storages lostDryRunbecauseAsCreateOptions()overwroteRawfrom zero-valued typed fields, while this one never carries it in the first place. Same observable, different cause.The storage is registered at
pkg/apiserver/apiserver.go:234, so the path is live. Effect ofkubectl --dry-run=serveron a Tap:Createreally creates the FluxOCIRepositoryincozy-system, andDeletereally removes thePackageSourceand possibly theOCIRepositorywith it.Worth stating for whoever closes this issue: fixing the three storages in #3937 does not close the class, and a reader checking "is dryRun honoured now" against the apps registry will get a yes that does not generalise.
Related, and cheap to add while the area is open: nothing stops the next aggregated storage from hand-building
client.CreateOptions{Raw: opts}and regressing this silently. There is no lint rule and no test that scans for it, so the only thing standing between the tree and a repeat is whoever reviews that PR remembering this one.The
Raw: optsshape does not work either, so the set is in worse shape than my earlier comment said.On the pinned
sigs.k8s.io/controller-runtime v0.23.1,CreateOptions.ApplyToCreate(options.go:303) copies the typedDryRunonly when it is non-nil, so a caller who fills in onlyRawleaves the typed field nil.AsCreateOptionsthen runso.Raw.DryRun = o.DryRunat options.go:287 and overwrites the populated raw marker with that nil. Four sites do it: 287, 371, 815, 913. The one at 371,AsDeleteOptions, also wipesGracePeriodSeconds,PreconditionsandPropagationPolicy.So
&client.CreateOptions{Raw: opts}is a no-op for dry-run, and on main today all three storages lose the directive:application rest.go:609 fresh &metav1.UpdateOptions{}, :502 fresh options in the force-create branch tenantsecret rest.go:262, :409, :430 Raw: opts, :404 fresh options in the force-create branch securitygroup rest.go:630, :806, :848 Raw: optsThe worse half is that the test meant to pin this cannot fail.
TestUpdateForceCreatePropagatesDryRunatpkg/registry/sdn/securitygroup/rest_test.go:755capturesco.Raw.DryRunfrom inside aWithInterceptorFuncscreate interceptor, and that interceptor runs before any conversion, so it reads the marker while it is still there and goes green over a production path that drops it.createOptionsFromUpdateat:494is written correctly; the wrapper eats its output afterwards.Two things for whoever picks up #3937.
application/rest.go:609is a plain empty options struct rather than theRawconversion, so a fix aimed only at the conversion layer will miss it. And the securitygroup test needs to assert downstream of the conversion instead of upstream of it, otherwise it keeps passing either way.
Summary
kubectl --dry-run=serveragainst the aggregatedapps.cozystack.io/v1alpha1API persists the change. The apps registry (pkg/registry/apps/application/rest.go) never inspectsoptions.DryRunonCreate,UpdateorDelete, so a request that the client explicitly marked as a simulation is executed for real. The same is true onmaintoday (noDryRunreference in the file).Environment
Cozystack v1.6.1 (
cozystack-api:v1.6.1-rc.1), Kubernetes v1.34.3, Talos v1.13.6. Observed 2026-08-28 during a security review of a lab cluster.Reproduction
Done twice with previously unseen annotation keys, read back with a different kubeconfig: the write reaches etcd every time. (Cleaned up afterwards with
kubectl annotate ... probe.example.org/dry-run-.)Expected
Per the Kubernetes API conventions, a custom
rest.Storageimplementation must honourDryRunitself: whendryrun.IsDryRun(options.DryRun)is true, run admission/validation and return the object that would be written without persisting anything.Deleteis the worrying case — by the same code path akubectl delete ... --dry-run=serveron aVMInstance/VMDiskwould delete the underlying HelmRelease for real (inferred from the code, deliberately not reproduced).Impact
Anyone relying on
--dry-run=serverto preview a change on anyapps.cozystack.iokind (VMInstance, VMDisk, Bucket, Postgres, ...) applies it instead, without any warning. It also breaks tooling that uses server-side dry-run for validation (e.g.kubectl diff, GitOps pre-flight checks, policy engines). No authorization boundary is crossed — the caller must already have write rights — but the semantics of a standard, documented client flag are violated.Suggested fix
In
pkg/registry/apps/application/rest.go, checkdryrun.IsDryRun(options.DryRun)inCreate,UpdateandDelete(andPatchif implemented separately): validate and build the HelmRelease as today, but return before any call to the underlying client. A conformance test asserting that a dry-run request leaves the backing HelmRelease untouched would prevent regressions.