feat: add a desired state aspect - #3598
Conversation
Signed-off-by: xstefank <xstefank122@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The newly added examples/tests assume metadata.labels is always non-null/mutable, which can cause NPEs and should be made null-safe (and documented accordingly).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new “desired state aspect” extension point to Java Operator SDK so operators can apply common cross-cutting mutations (e.g., labels/annotations) to all Kubernetes dependent resources’ desired state, and have those mutations participate in matching/update decisions.
Changes:
- Introduces
DesiredStateAspectAPI and wires it intoDefaultContext#getOrComputeDesiredStateForso aspects run once per dependent per reconciliation (with caching). - Extends
ConfigurationService/ConfigurationServiceOverriderto configure desired state aspects, and adjusts bulk dependent matching to use the processed desired state. - Adds unit + integration tests and documentation describing how to use desired state aspects to apply common metadata.
File summaries
| File | Description |
|---|---|
| operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectReconciler.java | Test reconciler wiring a dependent resource for desired-state-aspect integration coverage. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java | Integration test demonstrating global aspects adding labels to all managed resources. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectCustomResource.java | Test custom resource used by the desired state aspect sample/IT. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java | Updates test context construction to include a mocked configuration service exposing aspects. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java | Adds coverage ensuring aspects are applied in order and only once due to desired-state caching. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java | Adds tests for default/overridden/appended desired state aspects configuration. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java | Ensures bulk dependent matching uses the same desired-state pipeline (including aspects). |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DesiredStateAspect.java | New public functional interface documenting desired state aspects. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java | Applies configured aspects to freshly computed desired state before caching. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java | Adds override/append support for desired state aspects. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java | Adds desiredStateAspects() as a default empty list configuration hook. |
| docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md | Documents desired state aspects usage for adding common metadata to managed resources. |
Review details
Suppressed comments (1)
operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/desiredstateaspect/DesiredStateAspectIT.java:65
- Same issue as above: desired.getMetadata().getLabels() may be null, so this aspect can NPE on resources that don’t already define labels.
(desired, dependentResource, context) ->
desired
.getMetadata()
.getLabels()
.put(
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (desired, dependentResource, ctx) -> { | ||
| assertSame(testDependentResource, dependentResource); | ||
| assertSame(primary, ctx.getPrimaryResource()); | ||
| desired.getMetadata().getLabels().put("aspect", "first"); | ||
| }, | ||
| (desired, dependentResource, ctx) -> | ||
| desired.getMetadata().getLabels().put("aspect", "second"))); |
| (desired, dependentResource, context) -> | ||
| desired | ||
| .getMetadata() | ||
| .getLabels() | ||
| .put(MANAGED_BY_LABEL_KEY, MANAGED_BY_LABEL_VALUE), |
| Operator operator = new Operator(overrider -> overrider | ||
| .withDesiredStateAspects(List.of( | ||
| (desired, dependentResource, context) -> desired.getMetadata().getLabels() | ||
| .put("app.kubernetes.io/managed-by", "my-operator")))); |
| * <p>Implementations are expected to mutate the provided desired state in place and must be | ||
| * thread-safe as they can be called concurrently for different primary resources. |
Fixes #3094