Proposal to Allow Composition Functions to Set Claim Conditions

[dalton-hill-0]

Related to #5402

[negz]

I'm liking how this is looking, thanks @dalton-hill-0!

CC @phisco for a second opinion from the maintainer team.

[negz]

Out of scope for this design, but I could imagine building a generic function to expose this functionality to folks. Here's a rough sketch.

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: function-template-go
spec:
  compositeTypeRef:
    apiVersion: example.crossplane.io/v1
    kind: NoSQL
  mode: Pipeline
  pipeline:
  - step: patch-and-transform
    functionRef:
      name: function-patch-and-transform
    input:
    # Omitted for brevity
  - step: claim-status
    functionRef:
      name: function-result-extractor
    input:
      apiVersion: extractor.fn.crossplane.io/v1alpha1
      kind: Results
      results:
      - result:
          target: Claim
          severity: Warning
          message: Internal database error: ${error}
          status:
            type: DatabaseReady
            condition: "False"
            reason: InternalError
        triggers:
        - resourceName: rdsinstance
          type: StatusCondition
          status:
            type: Synced
            condition: "False"
            message: "Some AWS error: (?P<error>)"

The idea is you configure an array of results. Each result takes a series of triggers. If all triggers are true, the result is emitted. Each trigger uses regular expressions to match status conditions of composed resources.

[dalton-hill-0]

I added a WIP PR for the Crossplane changes (#5450).
Tests are not completed as the design is not yet agreed on.

[negz]

@dalton-hill-0 Sorry, I'm running even more behind on design reviews than I expected. I'll hopefully have time to focus on this this week though.

[dalton-hill-0]

@negz - FYI I just updated the PR to set ClaimConditions to a list of strings. Additionally, it contains a generic way of setting conditions that will emit events only if the condition has changed.

[negz]

@dalton-hill-0 I just added a comment to each of the two open comment threads. If you can address those, I'm happy to merge this design and we can focus on the implementation PR. Thanks!

[dalton-hill-0]

@dalton-hill-0 I just added a comment to each of the two open comment threads. If you can address those, I'm happy to merge this design and we can focus on the implementation PR. Thanks!

@negz - these have been addressed

[negz]

Awesome, thank you @dalton-hill-0!

[raphasle]

I stumbled upon this PR after realizing that functions can't write to the spec of the composite resource (as documented here). I used this functionality before using pipeline mode to write default values back to the claim (e.g. spec.version for a Postgres DB or spec.location for an Azure Storage Account) if the developer did not specify this optional parameter in the claim.
I know this is a bit off-topic here but maybe one of you can point me to a workaround example showing how to achieve this or to another design document saying why it should not be possible anymore to set fields in the spec (and metadata) section of the composite resource?

[negz]

@raphasle The short version is that it's hard to avoid bugs when we're syncing claim <-> composite <-> composed resource. See #4581.