refactor: Extract apply and update status steps - #974
Open
maltesander wants to merge 7 commits into
Open
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
Comment on lines
+147
to
+171
| /// Ensures the Secrets that the NiFi Pods mount but that the operator does not own exist, creating | ||
| /// any that are missing: the sensitive properties key and (for OIDC authentication) the admin | ||
| /// password. | ||
| /// | ||
| /// These are read-or-create client operations, so they cannot be part of the client-free `build()` | ||
| /// step. They are also deliberately not tracked in [`ClusterResources`], so they survive orphan | ||
| /// deletion and an existing Secret is never overwritten. | ||
| pub async fn ensure_secrets(client: &Client, cluster: &ValidatedCluster) -> Result<()> { | ||
| tracing::info!("Checking for sensitive key configuration"); | ||
| check_or_generate_sensitive_key( | ||
| client, | ||
| &cluster.cluster_config.sensitive_properties, | ||
| &cluster.namespace, | ||
| ) | ||
| .await | ||
| .context(SecuritySnafu)?; | ||
|
|
||
| if let NifiAuthenticationConfig::Oidc { .. } = cluster.cluster_config.authentication { | ||
| check_or_generate_oidc_admin_password(client, &cluster.name, &cluster.namespace) | ||
| .await | ||
| .context(SecuritySnafu)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
Member
There was a problem hiding this comment.
These Secrets don't need to be handled outside the normal reconciliation steps. We've hit the same situation in other operators and folded it into the regular pipeline instead: dereference the Secret in the dereference step, then in build emit it. Generate fresh contents when it's missing (or incomplete), otherwise re-emit the existing contents unchanged so the apply is a no-op. That way the Secret flows through build → apply as a normal, tracked cluster resource rather than a separate read-or-create side effect, and there's no special-casing around orphan deletion.
See e.g. stackabletech/druid-operator@d759183, where build_internal_secret() / reemit_internal_secret() do exactly this and the Secret becomes a regular resource added via the normal apply path.
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