refactor: Extract apply and update status steps - #974
Merged
Conversation
Make KubernetesResources generic over a marker that records whether the resources have only been built or have already been applied, and have build() return KubernetesResources<Prepared>. This prepares the extraction of the apply and update_status steps, where the Applied counterpart lets the type system prove that the cluster status is derived from the resource specifications returned by the API server rather than from the merely built ones. The Applied marker itself is added together with the apply step, since a marker struct that nothing constructs yet fails the dead_code lint.
Add deployed_product_version to ValidatedCluster, holding the bare NiFi version (for example 2.9.0) that is reported as status.deployedVersion. It is deliberately separate from product_version, which carries the full image app version label value (for example 2.9.0-stackable0.0.0-dev) used for the app.kubernetes.io/version label. This also fixes a latent panic. The reconciler parsed the bare product version with expect(), assuming it to be a valid label value. That holds for app_version_label_value, which resolve() truncates to the label value length limit, but not for product_version, which is copied verbatim from user input. Parsing now happens in the validate step and returns a ParseProductVersion error instead of panicking the reconcile task.
Move the resource application out of reconcile_nifi into a dedicated controller/apply.rs, mirroring the airflow and hbase operators. The Applier owns the ClusterResources, applies each resource kind through a single generic helper and returns KubernetesResources<Applied>, so the seven near identical apply loops collapse into one line each. Deleting orphaned resources moves along with it. Also move the two Secret side effects (the sensitive properties key and the OIDC admin password) into ensure_secrets in the same module. These are read-or-create client operations and are deliberately not tracked in ClusterResources, so they survive orphan deletion and an existing Secret is never overwritten. The reconciler's error enum collapses three variants into a single ApplyResources variant delegating to apply::Error.
Move the cluster status handling out of reconcile_nifi into a dedicated controller/update_status.rs, mirroring the airflow and hbase operators. The StatefulSet and cluster operation condition builders, computing the conditions and patching the status all live there now. update_status takes KubernetesResources<Applied>, so the type system proves the conditions derive from the resource specifications returned by the API server rather than from the merely built ones. Unlike the sibling operators it also takes the ValidatedCluster, because nifi additionally reports the deployed product version. reconcile_nifi is now a flat dereference -> validate -> build -> apply -> update_status pipeline, and its error enum delegates to the per step errors throughout.
Change object_meta to take the recommended Labels instead of a RoleGroupName it derives them from, matching the hbase operator. The call sites now pick the label variant they need, so resources that are not tied to a role group (or that must keep stable labels across version upgrades) can use the same helper instead of assembling their own metadata chain. No functional change, the labels are identical.
The CRD documentation claimed nifiPbkdf2AesGcm256 is the default, but the Default derive on NifiSensitiveKeyAlgorithm selects nifiArgon2AesGcm256, which is what validate falls back to when the field is left out. The security usage guide already documents Argon2 as the deployed default, so only the CRD field description was wrong. Regenerated extra/crds.yaml.
12 tasks
siegfriedweber
self-requested a review
August 11, 2026 08:11
siegfriedweber
requested changes
Aug 11, 2026
…y-and-update-status
… pipeline The sensitive-properties key and the OIDC admin password Secret were created by a read-or-create side effect (ensure_secrets) that ran before the apply step and was deliberately untracked, so it needed special-casing around orphan deletion. Both are now dereferenced, built and applied like every other cluster resource. Their contents are randomly generated and can therefore not be rebuilt, so an existing Secret is re-emitted with its fetched contents unchanged, which makes applying it a no-op and never rotates the key. The Secrets deliberately keep carrying no owner reference: the sensitive-properties key still decrypts the persisted flow after the cluster is recreated, and it may have been created by the user. For the same reason a user-provided key Secret (autoGenerate unset) is only required to exist and is never emitted, so the operator never writes to it. This empties out three modules, which are retired along the way: - security/sensitive_key.rs held nothing but two constants, which move to the builders that use them (the Secret key name to the Secret builder, the mount path to the shared build paths). - security/oidc.rs is split by concern: the nifi.properties fragment moves into the nifi.properties builder, the admin password Secret name next to the builder that emits that Secret. - security/mod.rs loses the build_tls_volume pass-through and the error-wrapping enum it existed for; the StatefulSet builder now calls tls::build_tls_volume directly and reports the actual failure. The operator now needs the patch permission on secrets.
siegfriedweber
requested changes
Aug 12, 2026
`ExistingSecrets::default()` reads as "the default Secrets" while it actually means "no Secret exists yet". The two test fixtures now spell both fields out.
The build step only ever creates a missing Secret, it never rewrites an existing one, so everything that can make a Secret unusable is now checked up front: the sensitive properties key Secret must exist when autoGenerate is off, and an existing Secret must carry its expected key. The latter was previously unchecked for the sensitive properties key and surfaced as NiFi Pods that could not start. It is rejected rather than filled in, because a regenerated key cannot decrypt the sensitive values of the already persisted flow. build_secrets() is infallible as a result.
Neither Secret is owned by the NifiCluster, so neither is ever orphan-deleted and there is nothing that re-emitting them on every reconcile run would protect against. The build step now emits a Secret only while it does not exist yet, which also keeps the operator from becoming the server-side-apply field manager of contents it must never rewrite.
The fixture properties the tests depend on are asserted where they are used instead of being implied by the shared minimal cluster, the Secret names are read off the cluster rather than hardcoded, and the assertions look Secrets up by name instead of relying on the order build_secrets happens to return them in. The existing-Secret helper no longer carries base64 contents that nothing reads.
siegfriedweber
previously approved these changes
Aug 13, 2026
siegfriedweber
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Prepared/Appliedtype-state markers toKubernetesResources, so the type system proves the cluster status is derived from the resource specifications returned by the API server rather than from the merely built onescontroller/apply.rs): theApplierowns theClusterResourcesand applies every resource kind through one generic helper, replacing seven near identical apply loops. Deleting orphaned resources moves along with itensure_secrets), covering the sensitive properties key and the OIDC admin password. Both are read-or-create client operations and stay untracked byClusterResources, so they survive orphan deletion and an existing Secret is never overwrittencontroller/update_status.rs), including both condition buildersobject_metato take the recommendedLabelsinstead of deriving them from aRoleGroupName, matching hbaseTwo things that are nifi specific and differ from the templates:
update_statusadditionally takes theValidatedCluster, because nifi also reportsstatus.deployedVersion, which the sibling operators do not havedeployed_product_versionand kept distinct fromproduct_version, which carries the image app version label value (2.9.0-stackable0.0.0-dev) used forapp.kubernetes.io/version. The status field must stay the bare product version (2.9.0)This also fixes a latent panic: the reconciler parsed the bare product version with
expect(), assuming it to be a valid label value. That holds forapp_version_label_value, whichresolve()truncates to the label value length limit, but not forproduct_version, which is copied verbatim from user input. It is now aParseProductVersionerror in the validate step instead of a panicking reconcile task.Lastly, a drive-by documentation fix: the CRD claimed
nifiPbkdf2AesGcm256is the default sensitive properties algorithm, but theDefaultderive selectsnifiArgon2AesGcm256, which is what the security usage guide alreadydocuments.
extra/crds.yamlregenerated.A preprocess step like the one in the opensearch operator was considered and deliberately deferred. opensearch introduced it together with a feature that needed it (injecting a role group into the spec), and neither airflow nor hbase
has one. nifi currently has no equivalent spec completion to do: every optional field carries
#[serde(default)]and the role config fragments are merged bywith_validated_config. The natural trigger to add it is upgrade awarenessreading the previous
status.deployedVersion.Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker