Skip to content

Proposal to Allow Composition Functions to Set Claim Conditions#5426

Merged
negz merged 7 commits intocrossplane:masterfrom
dalton-hill-0:fn-claim-conditions-doc
Mar 28, 2024
Merged

Proposal to Allow Composition Functions to Set Claim Conditions#5426
negz merged 7 commits intocrossplane:masterfrom
dalton-hill-0:fn-claim-conditions-doc

Conversation

@dalton-hill-0
Copy link
Copy Markdown
Contributor

Related to #5402

Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
@dalton-hill-0 dalton-hill-0 requested review from a team and negz as code owners February 23, 2024 22:12
@dalton-hill-0 dalton-hill-0 requested a review from bobh66 February 23, 2024 22:12
Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
… code examples

Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
Signed-off-by: dalton hill <dalton.hill.0@protonmail.com>
Copy link
Copy Markdown
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@negz
Copy link
Copy Markdown
Member

negz commented Feb 29, 2024

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
Copy link
Copy Markdown
Contributor Author

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

@negz
Copy link
Copy Markdown
Member

negz commented Mar 11, 2024

@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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Member

negz commented Mar 27, 2024

@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!

adding example of claimConditions field
@dalton-hill-0
Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you @dalton-hill-0!

@negz negz merged commit 9415fa2 into crossplane:master Mar 28, 2024
@raphasle
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

negz commented May 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants