[myasnikovdaniil]

This proposal defines installation defaults shared by all applications, applied only when a new instance is created.

My implementation work from designing this proposal is in the draft PR.

Before review

This is a new proposal and does not revise a merged design.

• If this revises a merged design proposal: it adds a decision record under that proposal's decisions directory, or says why none is needed.

DCO

• Commits are signed off.

[coderabbitai]

Important

• Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3dfa29be-94f4-4e5c-b266-65434496823e

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[IvanHunters]

Reading this as an architecture proposal. The core idea holds up: taking a snapshot of the chosen value at creation time is the right primitive for the case that motivates it (issue #3950), where applying a fallback at render time rewrites the KubevirtMachineTemplate name hash and recreates live workers. The merge table covers its edge cases, the testing section is thorough, and "Alternatives considered" is honest about the trade-offs. That is above the usual bar for a design doc.

Four things should be settled before merge, or at least spelled out in the doc. Smaller notes after that. I checked several of these against the linked prototype (#5107), so the references below point there.

[MAJOR] ConfigMap read turns application creation into a new fail-closed dependency

The doc says a missing ConfigMap means "no defaults" (fail-open), but a forbidden or failed read rejects creation (fail-closed). In the prototype that read is mgr.GetAPIReader(), uncached, on every create (pkg/apiserver/apiserver.go:258, installation_defaults.go:39), and it returns NewInternalError on any error that is not NotFound.

Two concerns:

1. A persistent read problem for one ConfigMap in cozy-system, say an RBAC change or an apiserver hiccup, now blocks creation of every application on the platform, even ones with no configured defaults. Creation did not depend on this object at all before. That is a steep availability cost for what the Security section itself calls "a convenience mechanism, not an enforcement policy."
2. The two "config is not there" cases do not line up: kubectl delete configmap fails open, a transient RBAC or etcd error fails closed. To an operator both are just "the config could not be loaded."

Please state the failure model you intend and justify fail-closed against that availability cost. And reconsider the uncached read on every create, since the config is create-only and changes rarely.

[MAJOR] Validation is deferred to create time, and the described validation is substantial and not yet built

"Saving the ConfigMap does not run these checks" means an operator can commit an invalid applicationDefaults and hear nothing about it until some later, unrelated user hits a confusing rejection. Open question #2 ("should malformed config for app X block every create, or only requests for app X") is not a detail. It is a core semantics decision, and it is still open. The validating webhook on the ConfigMap that would push the error back to the operator who caused it is explicitly out of scope, which defers the one mechanism that makes this safe to operate.

Separately, the "Validation of configured defaults" section describes schema-based validation against the ApplicationDefinition openAPISchema, with unknown-path rejection stricter than plain JSON Schema. The prototype does not do this yet. parseInstallationDefaults only checks JSON shape and reserved keys, and specSchema is built in NewREST but never used in rest.go, so right now the merged spec, defaults included, reaches the HelmRelease with no schema validation from the API and leans on admission and Helm instead. That is a lot of unbuilt work behind a short paragraph, worth flagging so reviewers do not under-weight it.

[MAJOR] Two independent implementations of the merge semantics

The merge rules are subtle: an omitted key copies, a non-empty object recurses, and explicit {}, scalars, null, and arrays are all preserved. The prototype implements them twice, Go fillInstallationDefaults (installation_defaults.go:104) and TypeScript fillMissing (lib/application-defaults.ts:23), because the dashboard submits displayed defaults as explicit fields. Same logic, two languages, and they have to stay in sync forever or the same user intent produces a different stored spec depending on whether it came through the dashboard or through apply/Terraform. Please note the risk in the doc and commit to shared fixtures or contract tests that run the same cases through both paths.

[MINOR] Persisted defaults carry no provenance

"The saved spec does not record their source or the ConfigMap revision." With create-only defaulting and no propagation, an operator who later finds a default was wrong cannot tell which instances got it, cannot separate user-chosen values from defaulted ones while debugging, and has no supported way to fix them. An annotation recording the applied-defaults snapshot or the ConfigMap revision would make this debuggable and leave room for a migration later. Worth discussing the trade-off instead of only noting the omission.

[MINOR] Secrets in the config are guarded by prose only

The Security section says contents "must not include credentials," but the examples (installerRepository, registryMirrors, factory URL) are exactly where operators tend to need registry auth. The ConfigMap is readable by authenticated dashboard users, and defaulted values get baked into every instance spec. Nothing enforces the rule. Consider detection or linting for secret-shaped values, or routing these fields through the existing secretRef mechanism.

[MINOR] {} versus omitted is a sharp edge for apply and YAML users

Telling "I did not set this" (omit) apart from "I want the default off" ({}) is non-obvious, and easy to get wrong in a hand-written manifest. It matches the merge table, but it belongs in the user-facing section too, not only in the table.

[MINOR] Quota and admission rejections get attributed to the user

Defaults merge in before the quota and admission checks, so an operator default that pushes size over quota will reject a user request that was valid as submitted. Reasonable choice, but the error lands on the user, and that deserves a note.

[MINOR / QUESTION] Field ownership under server-side apply

"Creation through apply must use the same create path" is stated, but apply is an upsert with field-manager ownership, not a clean create or update. The stored spec will carry defaulted fields the client never sent. Reads already surface schema defaults today, so part of this is pre-existing, but installation defaults are persisted rather than recomputed, and that part is new. How does the API tell a create from an update under a server-side apply PATCH, and who owns the defaulted fields on a later re-apply? A short treatment of SSA field ownership and Terraform drift would close the gap.

[NIT]

• The ## Decisions section is there but empty. If this is a new proposal with no decision record, say so.
• applicationDefaults and defaults.json have no version or schema field. With profiles and tenant overrides named as future work, a version marker is cheap insurance now.

I am for the direction and the create-time snapshot. My one ask is to hold merge until the failure model, the validation placement, and the dual-implementation risk are worked into the text, since those are decisions about the contract and the failure model rather than implementation details.

[IvanHunters]

The four blocking points from my earlier review are resolved in f6b8fd61, and resolved well:

• Fail-closed dependency: the failure model is now uniformly fail-open (continue without defaults on any read, parse, structure or schema failure), and the uncached read is now an explicit open question with the freshness trade-off stated.
• Validation placement: a malformed entry now skips defaults instead of blocking creation, so bad config for one application can no longer block unrelated creates. The blast-radius open question is gone, since the new behavior makes it moot.
• Dual merge implementation: gone. The merge lives only in Go and the dashboard previews through dryRun=All, and there is now an "A dashboard merge implementation" alternative that explains the choice.
• SSA create versus update: discussed now, with field ownership and reapplication called out for tests.

Thanks for the rework. Below are new points from a pass focused on architecture, existing-client compatibility, and migration on running clusters. None block the design. The ones under breaking clients and migration are what bite on adoption, so I would like them in the text.

[MAJOR] Dashboard previews run the full admission chain on every preview

Moving the merge to Go and previewing through dryRun=All is the right call, but the cost is not acknowledged. Each preview is a server-side dry-run create: defaulting, conversion to HelmRelease, validating admission (webhooks and VAP), and quota, which for a Tenant means the full ancestor walk. Two things to put in the doc:

• Load and latency move from the client to the apiserver, on every form interaction that refreshes the preview.
• Preview reliability depends on every relevant admission webhook supporting dry-run. A webhook with sideEffects other than None or NoneOnDryRun is rejected on dry-run, so previews fail for applications sitting behind such a webhook. The editor stays usable per the doc, so this degrades rather than breaks, but the failure mode is not mentioned.

[MAJOR] Enabling defaults silently changes behavior for existing clients

This is the main compatibility consideration, and it is not flagged as one. The moment an operator populates applicationDefaults, every existing Terraform module, CI pipeline, and GitOps repo that omits a field and leans on the chart default starts getting the installation default instead. "Client values win" only protects clients that set the field explicitly, and the clients that omit it are the whole point of the feature. That is intended, but it is a silent, cluster-wide change in meaning for every omitting client. Please flag it prominently, make the warning on a skipped or applied default definite rather than "may return a warning", and add an adoption note to dry-run before populating.

[MAJOR] Diff-based GitOps clients will see the persisted defaults

The SSA paragraph covers apply-PATCH field ownership. It does not cover controllers that normalize and diff desired against live, like ArgoCD or Flux applying Application CRs. Those compare the Git manifest with the field omitted against the live object with the field present and persisted, and can mark the application OutOfSync or try to prune the field. Some of this already happens today through read-time schema defaults, so clients that manage Application CRs declaratively handle part of it already, but installation defaults add persisted fields on top. Please state the expected interaction with diff-based controllers, not only with apply PATCH.

[QUESTION] What field manager owns the defaulted fields

apply-create routes to Create so defaults are applied, and apply-update routes to Update so they are not. That part is correct. But the doc defers ownership to "the aggregated API's existing field-ownership behavior" and only commits to testing it. What a client actually sees, for instance what terraform plan shows after the API injects a field the config does not contain, depends on whether the aggregated API tracks managedFields on Application objects at all. Please state the expected ownership and the resulting client behavior, not just that tests will cover it.

[MAJOR] Mixed-version window during upgrade can make creates nondeterministic

cozystack-api rolls one replica at a time. If an operator populates applicationDefaults before every replica is on the new version, the same create request applies defaults or not depending on which replica serves it, and a dashboard preview through an old replica will not match a create served by a new one. Rollout says ship together and keep it empty by default, but there is no explicit instruction to leave the config empty until every cozystack-api replica is upgraded. Please add that.

[MAJOR] A mistyped plural is silently dormant

An unknown plural is retained for a future catalog and matches nothing, with no error or warning. Pair that with awkward plurals like kubernetesnodeses, and an operator adopting this can set a default, see no error, and get no effect. That is a rough first run. Consider a way to see which entries matched a live application and which are dormant, or a warning on an unknown plural.

[MINOR] Rollback claim is a bit strong under a chart downgrade

"Values already supported by their charts" holds while charts only move forward. If a default targets a field valid only in a newer chart and the platform is later rolled back, a release carrying that value can fail to render against a chart with a strict values schema or a renamed field. It is narrow, and it depends on whether the charts use strict values schemas, but the sentence could be qualified.

[QUESTION] Coexistence with the legacy registries setting

The Talos example sets registryMirrors through this mechanism while the old global registries renderer stays, which is a stated non-goal. On a cluster already using registries, two mechanisms now influence image pulls. How do they interact, and is there guidance to avoid configuring the same thing twice?

[Timofei Larkin (lllamnyp)]

Thanks for this. The create-time snapshot is the right primitive, and the generic, plural-keyed shape is a real improvement over an application-specific defaulting path in the API server. It also has good Kubernetes precedent: DefaultStorageClass and LimitRanger stamp defaults into the object at admission, and OpenShift's project template is create-only for the same reason. Citing them in the proposal might help readers.

I've tried not to repeat points from the earlier reviews. The main thing I'd ask for is a serious look at MutatingAdmissionPolicy as the mechanism (see the comment on "Alternatives considered"). It is a native create-time mutation and may let this ship as applied YAML, with no new Go in the create path. Running it inside an aggregated API server has caveats that need a short spike, though. Beyond that, I'd like two things settled in the text: the relationship to cozystack/cozystack#4897, and the failure model now that a missed default is permanent.

Several of the comments below assume the current Go design. If the MutatingAdmissionPolicy route wins, some of them change shape or go away.

[Timofei Larkin (lllamnyp)]

Nit: it would help to say briefly why these values stay out of cozystack-values: a change there reconciles every release, and the values would reach every chart. It would also help to say that the ConfigMap is an internal rendering detail, and that applicationDefaults is the supported interface.

[Timofei Larkin (lllamnyp)]

A few things to pin down here, assuming the Go design stays:

• The check may not need a bespoke validator. One concrete approach: copy the structural schema with required removed, use pruning to detect unknown fields, then run standard validation.
• CEL rules (x-kubernetes-validations) aren't well defined on a partial object. It may be better to skip them for the entry and rely on admission of the merged spec.
• When an application upgrade changes its schema so that a stored default no longer validates, defaults for that application silently stop applying. Some operator-visible signal, such as an Event on the ConfigMap or a metric, alongside the dormant-plural signal suggested earlier, would make that discoverable.

[Timofei Larkin (lllamnyp)]

Fail-open is the right call for configuration errors, and it removes the blast-radius problem raised earlier. Combined with create-only defaulting, though, a skipped default can't be recovered. The instance is stored without it and follows the developer default from then on. For the motivating case, that means an air-gapped installation ends up with a pool pointing at the public image factory: the outcome #3950 asks to prevent, and the reason #4897 returned 503 on a failed read.

If the Go design stays, I'd suggest resolving the first open question (line 114) as part of this. Read the ConfigMap from an informer that has synced before the API serves; the API server already does this for Secrets. Transient read failures then effectively go away. Then keep fail-open for invalid entries, but make the warning mandatory, including on dry-run, so the dashboard can show it.

[Timofei Larkin (lllamnyp)]

Recursing into non-empty objects also applies to free-form dictionaries such as talos.registryMirrors. Take a pool that declares its own docker.io mirror. It would also receive the platform's mirrors for other registries. If the platform configures docker.io too, the pool's entry would pick up the platform's fields, such as skipFallback, producing an entry neither side wrote. #4897 settled on "the pool's mirrors replace the platform's as a whole".

Would you consider treating dictionary-typed values (additionalProperties) atomically, as arrays already are? Kubernetes' x-kubernetes-map-type: atomic is precedent for making this schema-driven.

[Timofei Larkin (lllamnyp)]

Minor: applications whose HelmRelease is written directly, rather than created through the API (for example by the ComputePlane module or by platform migrations), won't receive installation defaults. Worth a line in the failure and edge cases list.

[Timofei Larkin (lllamnyp)]

The create response goes through the same conversion as a read, and that conversion applies schema defaults. So the dry-run preview contains developer defaults as well as installation defaults. Because the dashboard submits every value it displays, dashboard-created applications store developer defaults too (for example talos.version), while the same request through kubectl leaves them live.

This matches how the form behaves today. It does conflict with "This proposal does not freeze developer chart defaults", and it means the same intent produces different stored specs depending on the client. Could the proposal either say so explicitly, or have the dashboard submit only the fields the user changed and let the real create apply installation defaults again?

[Timofei Larkin (lllamnyp)]

The generated Go types in api/apps/v1alpha1/*/types.go declare fields such as storageClass, size and talos without omitempty. Any client built on them always sends "", a zero quantity or a full object. Under these rules those are user values, so installation defaults never apply to such clients, for the very fields used in the examples.

Worth stating here, and perhaps tracking a follow-up to make optional fields omitempty in the generator.

[Timofei Larkin (lllamnyp)]

This example covers the same ground as cozystack/cozystack#4897. That PR adds KubernetesNodes-specific create-time stamping to the API server (rest_create_defaults.go, keyed on the kind name), and moves platform registry mirrors to render time via _cluster.

Could the proposal state explicitly that it supersedes the API-server part of #4897, so core never carries an application-specific defaulting path?

Could it also say which mechanism owns registry mirrors? With #4897's kubernetesRegistries, a mirror change reaches every pool and rolls it. As an applicationDefaults entry, it reaches only new pools. Both are reasonable, but for different intents. A sentence such as "settings that must track the platform belong in platform values consumed at render; this mechanism is for initial choices" would draw the line.

Linking #4897 and the prototype (cozystack/cozystack#5107) from the Context section would help readers too.

[Timofei Larkin (lllamnyp)]

Could this section say explicitly that installation defaults are not an enforcement mechanism? Operators who need to require a value should use a ValidatingAdmissionPolicy or a webhook on the application resource. The create path runs admission on the merged spec, so the two compose cleanly.

[Timofei Larkin (lllamnyp)]

I'd like to see MutatingAdmissionPolicy (MAP) considered seriously here: either as the mechanism, or as a written rejection after a short spike. It is new enough (served as admissionregistration.k8s.io/v1 from Kubernetes 1.36) that it's easy to overlook. It looks like a native fit, though: what this proposal implements in Go is a create-time mutation, and that is exactly what MAP is.

How it would map onto this design:

• The Cozystack API server already runs the generic admission chain before the REST Create. A policy matching apps.cozystack.io resources on CREATE sees exactly what the client sent, with no schema defaults applied yet. Everything downstream (null normalization, quota, validating admission, conversion to HelmRelease) then sees the mutated object. That is the ordering this proposal wants.
• A server-side apply that creates an object reaches admission as CREATE, so the apply case needs no special handling. Updates are excluded simply by not matching UPDATE.
• Admission runs on dry-run, and policies have no side effects, so the dashboard preview design stays as it is.
• failurePolicy is set per policy. That turns the fail-open vs fail-closed question into an explicit operator choice.
• matchConditions, namespace selectors and a per-namespace paramRef give conditional and tenant-scoped defaults without further design. Both are non-goals here today.
• There is no Go in the core path at all. The mechanism is YAML that the platform chart, or an operator, applies.

A JSONPatch mutation guarded by has() expresses "fill only what is missing":

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingAdmissionPolicy
metadata:
  name: postgreses-installation-defaults
spec:
  matchConstraints:
    resourceRules:
    - apiGroups: ["apps.cozystack.io"]
      apiVersions: ["v1alpha1"]
      resources: ["postgreses"]
      operations: ["CREATE"]
  failurePolicy: Ignore
  reinvocationPolicy: Never
  mutations:
  - patchType: JSONPatch
    jsonPatch:
      expression: >
        !has(object.spec)
          ? [JSONPatch{op: "add", path: "/spec", value: {"storageClass": "replicated", "size": "20Gi"}}]
          : (has(object.spec.storageClass) ? [] : [JSONPatch{op: "add", path: "/spec/storageClass", value: "replicated"}])
            + (has(object.spec.size) ? [] : [JSONPatch{op: "add", path: "/spec/size", value: "20Gi"}])
---
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingAdmissionPolicyBinding
metadata:
  name: postgreses-installation-defaults
spec:
  policyName: postgreses-installation-defaults

CEL has no recursion, so one generic policy can't deep-merge an arbitrary parameter object. The platform chart could instead compile applicationDefaults into one policy per plural at render time. That keeps the operator-facing shape this proposal defines, and expresses the merge rules, including "an explicit {} stops defaulting", in the generated conditions.

The caveats are mostly about running MAP inside an aggregated API server, and they are what a spike would need to settle:

1. Kubernetes version. The plugin in the vendored k8s.io/apiserver (v0.37.1) watches admissionregistration.k8s.io/v1, which clusters older than 1.36 don't serve, and it refuses writes until its informers have synced. That is why the Cozystack API server disables the plugin by default today (disableUnsyncableAdmissionPlugins in pkg/cmd/server/start.go). Adopting MAP means a 1.36 floor for the management cluster, or a version gate. That has to be weighed against the Kubernetes versions Cozystack supports.
2. RBAC. The API server needs list/watch on policies, bindings and any param kind.
3. Type resolution. Before applying any patch, JSONPatch included, the dispatcher looks up a type converter for the matched kind in the cluster's /openapi/v3, which it polls every few seconds. If the kind isn't there yet, the dispatcher returns a 503 status error, and in v0.37.1 that error bypasses failurePolicy. So a policy matching an application whose OpenAPI hasn't yet been published through aggregation fails that create closed, even with failurePolicy: Ignore. Worth testing across an API server restart and when a new ApplicationDefinition is added.
4. Patch type. In Go, Application.spec is raw JSON. The published schemas contain x-kubernetes-preserve-unknown-fields nodes and atomic lists, which ApplyConfiguration mutations reject or handle poorly. JSONPatch looks like the safe choice. The round trip back into the typed object should be covered by a test.
5. The policy machinery has hit these aggregated resources before. The VAP status type-checker crashed on boolean additionalProperties in their schemas (kube-controller-manager CrashLoopBackOff: cozystack-api publishes additionalProperties:true on Tenant schema, panicking the VAP status type-checker cozystack#2863), so the spike should run a MAP against every shipped ApplicationDefinition.

If the spike shows these are manageable, MAP would take the Go merge, the custom partial-schema validator and the ConfigMap read out of the create path. It would also make the failure model and conditional defaults operator-configurable. If the caveats are not manageable, it would help to record the reasons in this section, so the question doesn't keep coming back.

[myasnikovdaniil]

IvanHunters thanks, updated the proposal. Answers to the second review:

• Dry-run cost: the dashboard now sends one dry-run create when the order page opens and no previews on edits, Deploy doesn't wait for it. Cost and the requirement for webhooks to support dry-run are in the Dashboard section.
• Adoption: Overview now says omitted fields of a configured kind take installation values cluster wide, and the warning on skipped defaults is definite. No warning on applied values, it is normal operation and the create response shows what was stored. Dry-run before populating is not possible, it reads the live config, so the doc describes checking after the entry is applied (with a name not in use, existing name returns AlreadyExists).
• GitOps and field ownership: stated in Client behavior. Application objects have no stored managed fields, apply to an existing app goes through update and keeps stored defaults, apply never removes fields. Argo CD, Flux and Terraform were not tested and the doc says so.
• Mixed versions: not adding it. Rolling update has this window for any change, a create in it just misses defaults like in the fail-open case, and config is empty by default.
• Dormant plural: documented how to list plurals and check an entry with dry-run, no startup validation, too much code for a typo.
• Rollback: all 23 values schemas of apps in the repo allow unknown keys, so a stored value for a field an older chart doesn't know is ignored, not rejected. Left the sentence as is.
• Legacy registries: nothing in the repo reads the Secret it renders, talos.registryMirrors of the pool is the only active path, so there is nothing to coexist with. Removing it stays out of scope.
• defaults.json version field: not added, the ConfigMap is internal and applicationDefaults is the interface.

[myasnikovdaniil]

Timofei Larkin (@lllamnyp) thanks for the review. Updated the proposal.

• MAP: rejected in Alternatives. The plugin is disabled in cozystack-api (no RBAC to list policies, not served before 1.36), CEL can't recurse so the chart would generate a policy per plural, and the 503 for a kind missing in OpenAPI bypasses failurePolicy. This is from reading the code, no spike was run.
• cozystack-values and ConfigMap: both sentences added to Configuration.
• Validation: the prototype already does what you describe, a copy of the schema without required and the standard validator, with unknown fields caught by additionalProperties false where the schema declares neither it nor preserve-unknown-fields. CEL is not evaluated for the entry, now in the text. Schema drift gets the mandatory warning on create plus a log line, no Event or metric, and the doc says to check with dry-run after application upgrades.
• Failure model: the config is read from an informer that syncs before the API serves, fail-open stays for invalid entries, the warning is mandatory and returned on dry-run too. The dashboard doesn't show API warnings yet, noted in the doc.
• Dictionaries: atomic now, same as arrays.
• Direct HelmReleases: added to edge cases.
• Developer defaults: documented in Client behavior, same intent gives different stored specs depending on the client. Submitting only changed fields from the dashboard stays a possible later change.
• omitempty: stated in the doc, the generator change deserves its own issue.
• feat(platform, kubernetes): platform-wide defaults for tenant Talos worker settings cozystack#4897: its API-server part is not carried forward, the plural-keyed path covers that case, noted in Alternatives. Mirrors set here reach new pools only, existing pools change theirs by editing the spec, there is no render time mirror channel in this proposal. Prototype is feat(api): add installation defaults for new applications cozystack#5107, draft and PoC.
• Enforcement: added to Security, VAP or webhook for required values.
• Precedents: added to Context.

[myasnikovdaniil]

IvanHunters a correction to what I wrote above: Deploy does wait for the preview to answer. The form opens as soon as the dry-run answers, an error included, and a failed preview doesn't block Deploy. The 8 seconds is only a cap for a request that gets no answer at all, after that the preview counts as failed. Updated the proposal text to say so.

[IvanHunters]

The design is in good shape now. My four original blockers and most of the second round are resolved, several with concrete answers I had not checked myself: all 23 application values schemas allow unknown keys, so rollback is safe; nothing reads the legacy registries Secret, so there is nothing to coexist with; and the mixed-version window degrades to the documented fail-open skip rather than harmful nondeterminism. The single dry-run on page open and the cached watch-backed reader also settle the earlier cost and availability concerns. Approving.

Five notes, none blocking. A and B are the two I would most like to see in the text before implementation.

[MAJOR] Overview overclaims against the omitempty caveat

Overview says every client that omits a field, including kubectl, Terraform, CI jobs and GitOps repositories, takes the installation value. Client behavior then carves out Go clients built on the generated types, which send "", 0 or a full object because the types lack omitempty, so installation defaults never apply to those fields. Those two statements contradict each other for a whole class of clients. Two things to resolve in the text:

• If the Cozystack Terraform provider is generated from those types, it lands in the no-omitempty class and gets no defaults, so the Overview's Terraform claim is false. If the intended path is kubernetes_manifest over raw YAML, say so. Either way the Overview should be qualified.
• For the motivating case, which client actually creates KubernetesNodes pools? If it is a typed Go client, the feature is inert for its own headline scenario until the omitempty generator change lands, and that change is a separate out-of-scope issue. That makes omitempty a precondition, not a footnote, so please state which clients the feature serves on day one.

[MAJOR] "Recurse into declared properties, atomic otherwise" is undefined for hybrid schemas

The merge now recurses into objects with declared properties and replaces dictionaries (additionalProperties) and free-form objects (x-kubernetes-preserve-unknown-fields) as whole values. Two consequences:

• The merge now has to walk the openAPISchema alongside the values to decide recurse versus atomic, resolving $ref and allOf as it goes. That is more than the structural check the prototype has today, so it is new work, not just a rule.
• The declared-properties-versus-dictionary split is binary, and a node can be both. An object with properties and x-kubernetes-preserve-unknown-fields: true is common in CRDs, and properties with additionalProperties is also possible. For such a node the two rules conflict: recurse or replace? Please define the rule for the mixed case, or confirm the application structural schemas cannot produce it.

[QUESTION] The create-only boundary leaks on the first update

"The first update by any client, apply or patch, also saves the developer defaults the API returns on reads into the stored values." This is existing API behavior, not something this PR introduces, but it works against the boundary the whole design is built around. Installation defaults are honestly create-only and are not re-applied on update, yet the first edit of any field silently freezes every current developer default into the stored spec, and after that a chart-upgrade default change no longer reaches that instance. Worth saying plainly that the create-only boundary holds for installation defaults but not for developer defaults, so the clean new-versus-existing separation does not fully hold in practice.

[MINOR] The dashboard pins developer defaults, so stored specs diverge by client

Because the dashboard submits every displayed value, an application created there stores developer defaults too, while the same request through kubectl does not. The doc is honest that this matches today's dashboard and that the same intent yields different stored specs by client. "Submit only the fields the user changed," listed as a possible later change, is the fix that makes the two paths consistent and stops the dashboard path from defeating the chart-upgrade propagation the Alternatives section leans on. I would raise it from a possible later change to a stated dependency or recommendation.

[MINOR] Note the cache staleness window

The watch-backed reader removes the uncached read and the transient-error failure, but a saved change reaches creates only shortly after. Instances created in that window are pinned to the old default with no signal, since fail-open is silent, and a silently desynced watch would do the same. The risk is low because the informer relists, but a sentence on the staleness window and its effect on pinning would be worth adding.

[myasnikovdaniil]

Thanks. I fixed two places in the text.

Overview no longer lists Terraform, Client behavior already says fields a client sets itself count as user values. The provider leaves the Talos fields (factory URL, installer repository) out when unset, so the KubernetesNodes case works there, while its own defaults for storage class and disk size do not take installation values.

The text said the API serves only after the cache has synced, but the wait is bounded and a create that cannot read the cache continues without defaults. Text matches the code now, this covers the staleness note too.

• B. No application schema has an object with both properties and a dictionary (checked all 31), so I keep the rule as is. Code replaces such a node whole if one appears.
• C. It is already in Client behavior, the first update saves developer defaults.
• D. Dashboard stores developer defaults today and the doc says so. Submitting only changed fields stays a possible later change, not needed here.

[Timofei Larkin (lllamnyp)]

Thanks for the thorough rework. Every point from my review is addressed in the text, so I'm approving. One non-blocking note on the MutatingAdmissionPolicy section is inline.

On cozystack/cozystack#4897: since this proposal supersedes its API-server part, could you drop rest_create_defaults.go and its Create hook from that PR, or close it in favour of implementing this proposal, so the two paths don't both land?

Small wording fix in the Dashboard section: "The 8 seconds is a cap…" refers to a timeout the text never introduces. Maybe: "The preview has an 8-second timeout for a request that gets no answer at all…".

[Timofei Larkin (lllamnyp)]

Thanks for writing this up. I agree it doesn't fit today; the 1.36 floor on its own settles that. Two small asks so the section ages well:

• It lists only the costs. Could it also note what MAP would bring, so a later reader can weigh it? That would be a failurePolicy per policy, matchConditions for conditional defaults, and namespace-scoped parameters for tenant-level defaults.
• Since applicationDefaults is the supported interface and the ConfigMap is internal, the backend could move to MAP later without changing what operators write. Could the section say so, and name the trigger for revisiting it: the minimum management-cluster Kubernetes version reaching 1.36?

One more point that supports your conclusion: with dictionaries now replaced whole, a policy generated by the platform chart would need to know which nested objects are dictionaries. That comes from each application's schema, which the platform chart can't see when it renders, particularly for external catalogs. Worth a sentence too.

[myasnikovdaniil]

Thanks, fixed the wording and the MAP section. Closed #4897 in favour of this proposal. One new sentence in the merge rule too: objects marked x-kubernetes-map-type: atomic are replaced whole. Push dismissed your approval, take a look please.