Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composition Functions Beta: Long-running Functions #4306

Merged
merged 15 commits into from
Jul 17, 2023
Merged

Conversation

negz
Copy link
Member

@negz negz commented Jul 10, 2023

Description of your changes

This pull request introduces an updated design for Composition Functions. Under this design Functions are long-running processes, like Providers. Crossplane makes RunFunctionRequest RPCs to a Function using gRPC.

This updated design also sketches out an improved Function development experience, as well as a few hypothetical 'generic' Functions. A generic Function is a Function that works with any XR. Generic Functions extend Crossplane with new ways to express Composition logic without actually needing to develop a Function of your own.

My intention is that if this design is approved, we'll implement it as v1beta1 support for Functions in Crossplane v1.14 (due in October).

I have:

  • Read and followed Crossplane's contribution process.
  • Added or updated unit and E2E tests for my change.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

@negz negz requested a review from a team as a code owner July 10, 2023 08:14
@negz negz requested a review from hasheddan July 10, 2023 08:14
@negz
Copy link
Member Author

negz commented Jul 10, 2023

If this design is accepted I'll need to update the plan laid out in the beta tracking issue. A lot of the issues it tracks are specific to the xfn Function runner, which under this design would become just one way to build a Function.

I believe the very high-level implementation plan for this design would be:

  • Move xfn out of tree - e.g. to crossplane/function-runtime-oci - and remove it from the Helm chart
  • Add Function support to the package manager
  • Remove the FunctionIO type and update the RunFunctionRequest and RunFunctionResponse RPC types
  • Refactor the PTFComposer to accommodate the design (which I think would be a simplification)
  • Add the illustrated commands to kubectl crossplane (or a new CLI?)
  • Start with one SDK (probably Go) and one generic Function (probably Go Templates).

@negz negz requested review from jbw976 and turkenh and removed request for hasheddan July 10, 2023 08:27
* Reviewers: Crossplane Maintainers
* Status: Draft
* Owners: Nic Cope (@negz)
* Reviewers: Hasan Turken (@turkenh), Jared Watts (@jbw976)
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose these reviewers because:

  • @turkenh has context from reviewing the original, alpha design
  • @jbw976 will be leading a new developer experience effort, which I feel relates closely to Functions


// The resource's connection details.
map<string, bytes> connection_details = 2;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Under the alpha design a Function can return a composed resource that specifies P&T style readiness checks and ways to derive connection details (e.g. connectionSecretFromField etc). I've removed that under this design. I don't think I've seen anyone use it, and I don't think we actually need it. We can always add it back if there's a demand.

It's possible for a Function to just derive and write whatever XR connection details it needs, and to set the XR's as Ready whenever it feels it is. I'm pretty sure @turkenh identified we didn't need this in the original design and I think I agreed but must have forgotten and built it anyway? See #2886 (comment).

with ControllerConfig, but don't yet have a suitable alternative. Rather than
propagating a ControllerConfig-like pattern I propose we prioritize finding an
alternative. I intend to open a separate, simultaneous design to address this
since it will affect Provider packages as well as Functions.
Copy link
Member Author

@negz negz Jul 10, 2023

Choose a reason for hiding this comment

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

Edit: Here's the proposal:

Copy link
Member

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Looking great, I like the direction we are heading toward!
Especially the possibility of converging to the same machinery with the Patch-and-Transform as a Function. instead of treating them as two different ways to compose resources.

design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

Awesome work @negz!! I really like the goals of this document around making functions easier to write and to use 🙇 🤩

A few questions for your consideration to help explain a few gaps in my understanding, nothing major blocking from my side!

design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
design/design-doc-composition-functions.md Show resolved Hide resolved
Comment on lines +236 to +260
Despite the name, a 'Function' is actually more like a 'function server'. Under
this proposal, Functions are long-running processes. When you install one, the
package manager deploys it using a Kubernetes Deployment - the same way it would
deploy a Provider.
Copy link
Contributor

@phisco phisco Jul 11, 2023

Choose a reason for hiding this comment

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

What about calling the "function server" FunctionRunner and Function an actual configuration to be run with a function runner? I mean, a function to me is something that takes an input and produce some output, so something like the following could work:

---
apiVersion: pkg.crossplane.io/v1beta1
kind: FunctionRunner
metadata:
  name: go-templates
spec:
  package: xpkg.upbound.io/negz/go-templates:v0.1.0
status:
  # The gRPC endpoint where Crossplane will send RunFunctionRequests.
  endpoint: https://go-templates-9sdfn2
---
apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
  name: my-xpostgresql-go-templates
spec:
  functionRunnerRef:
    name: go-template
  config:
    apiVersion: example.org/v1
    kind: GoTemplate
    source: Remote
    remote: git://github.com/example/my-xpostgresql-go-templates
---
apiVersion: apiextensions.crossplane.io/v2alpha1
kind: Composition
metadata:
  name: example
spec:
  compositeTypeRef:
    apiVersion: database.example.org/v1
    kind: XPostgreSQLInstance
  functions:
  - functionRef:
      name: my-xpostgresql-go-templates

which could be inlined as follows:

---
apiVersion: apiextensions.crossplane.io/v2alpha1
kind: Composition
metadata:
  name: example
spec:
  compositeTypeRef:
    apiVersion: database.example.org/v1
    kind: XPostgreSQLInstance
  functions:
  - functionRunnerRef:
      name: go-templates
      config:
        apiVersion: example.org/v1
        kind: GoTemplate
        source: Remote
        remote: git://github.com/example/my-xpostgresql-go-templates

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, a function to me is something that takes an input and produce some output

I think that's the "trick" here. We're really trying to frame these things (that are in practice more like servers) as "functions". We want folks to feel like they're building, installing support for, and calling "just a function" - not a server or runner. The fact that we make it a server at build time is a bit of an "implementation detail".

So to that end I think I prefer sticking with just "function" rather than introducing a runner type.

I see the (optional) config block in a Composition as kind of like an argument to a function call. In pseudocode Crossplane is making a call like this:

run_function(xr, composedResources, config) (xr, composedResources)

I think there will be a desire to avoid repeating the same config across multiple function calls, but I'd rather do that by allowing Functions to deliver a Config CRD, which could be instantiated and referenced. This has the added benefit of allowing Functions to deliver fully (OpenAPI) schemafied config types that the API server could validate.

Copy link
Member Author

Choose a reason for hiding this comment

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

@turkenh @jbw976 what do you think here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my confusion comes from the fact that I would think to be writing more “runtime” based functions than full-fledged ones, hence my point of separating functions and function runners (a.k.a. runtimes).

Maybe it’s because I’m thinking of something similar to this which I used in the past, but as a user I would expect to be writing more quick “scripts” in Python, Starlark or some other scripting language, rather than really complex “functions” that would require to be wrapped in a grpc server on their own.

But I feel your focus is instead more on full-fledged functions, so it makes sense to consider “runtime” ones as an exception/hack w.r.t. the concept of a function, in which one of the arguments to the function being called is actually custom logic/code, and therefore don’t deserve being first-class citizens of the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I share @phisco's view on Functions and FunctionRunners. Also calling it config suggests that the function is the thing that has consumed the config:

function(resources) := functionRunner(config)(resources)

But this is clearly cosmetics.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to avoid introducing a new FunctionRunner concept to our users and keep it as implementation details as long as it makes sense.

I agree with @phisco's concerns around the need for reusable config's for functions, however, would like to see it as Function (as package) + FunctionConfig (as CRD), which is consistent with what we have today with Provider (as package) + ProviderConfig (as CRD). (This could also help with the passing credential problem in #3718 /cc @ezgidemirel).

Copy link
Member Author

Choose a reason for hiding this comment

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

After some discussion with @turkenh I've renamed config to input. This better communicates what this type is for. It doesn't configure the Function type in any global way - it just specifies optional input passed with the Function call.

Copy link
Contributor

@phisco phisco Jul 13, 2023

Choose a reason for hiding this comment

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

Wouldn’t this mean that a function would have two types of inputs? The config and the actual input coming from the P&T pipeline or the previous function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn’t this mean that a function would have two types of inputs? The config and the actual input coming from the P&T pipeline or the previous function?

Yes - that's what I was trying to communicate before. The input to a "function call" is like function(xr, composed_resources, input).

The mental model of a call being more like runner(input)(XR, composed_resources) does conceptually make sense to me, especially given that we think some "inputs" will be repeated a lot - i.e. N Compositions might all call the same function with the same input (fka config).

I still prefer to not expose "runner" as a distinct concept just because:

  • There's fewer concepts to think about overall. You just install ("define") a Function, then call it (from a Composition).
  • Similarly, there's fewer API types to worry about. In many cases you just install a Function (often as a dependency of your Configuration) and call it from your Composition. You don't need to install a FunctionRunner, then define a Function, then call that from your Composition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think most people are going to use “runtime” based functions, at least that’s what I would do. So the two options are actually Function (+ PackageRuntimeConfig) + FunctionConfig or FunctionRunner (+ PackageRuntimeConfig) + Function. I still think the latter would be clearer to me, and it’s actually more similar to the original proposal, just with less focus on OCI images. However as we said it’s mostly cosmetic, so I’m ok marking this thread as resolved given that it looks like there is definitely consensus around the proposed solution.

@negz negz requested review from jbw976 and turkenh July 12, 2023 02:29
@negz
Copy link
Member Author

negz commented Jul 12, 2023

@turkenh @jbw976 I've pushed a few commits to address feedback and resolved the related threads. Please take another look and let me know what you think.

XRs rely on poll-triggered reconciliation to promptly correct drift of their
composed resources. The poll interval can be set using the `--poll-interval`
flag. The XR controller does not know what kinds of resources it will compose
when started, so it cannot start a watch for them.
Copy link
Contributor

@sttts sttts Jul 12, 2023

Choose a reason for hiding this comment

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

The function invocation should not depend on the composed objects.

We know that the first function in the pipeline only depends on the XR, claim (we make that available too, don't we?) and the composition, and with that the whole pipeline.

What we need is a way to detect drift of the composed objects in order to trigger another run of the pipeline on demand. Technically, we don't even need a cached image for the trigger (we have to detect deletions and updates to the spec of composed objects).


With that said, it would be good to clearly call out the assumptions a function author can depend on. Being called every 60s should not be one of them IMO. Determinism for a defined set of inputs should be a requirement for every function. That keeps the freedom for us to improve the implementation later.

Copy link
Contributor

@sttts sttts Jul 12, 2023

Choose a reason for hiding this comment

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

I learned today that compositions are not well-founded in the sense that they can depend on siblings from the live system through ToCompositeFieldPath. We are actually not really building just compositions, but at the same time they are transitions of a state machine. Mind blowing. We don't call that out anywhere properly or I didn't find it.

Our innnocent looking sentence in the docs:

FromCompositeFieldPath and ToCompositeFieldPath patches can also take a wildcarded field path in the toFieldPath parameter and patch each array element in the toFieldPath with the singular value provided in the fromFieldPath.

In that sense the inputs of a function include the live system MRs, and maybe some day we watch them too. Would be better IMO than the tag+ttl idea you are mentioning further down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't both work?

There's a section on potentially watching composed resources under "Future Considerations". Also #4316.

Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all of my feedback @negz, this is looking good to me with the feedback you've incorporated! 💪

negz added a commit to negz/crossplane that referenced this pull request Jul 13, 2023
This one-pager proposes a lightweight implementation of the PRI
proposal, along with an alternative for ControllerConfig.

I believe this will be relevant for the in-flight Composition Functions
beta design, as well as for Providers.

crossplane#3601
crossplane#2671
crossplane#4306

Signed-off-by: Nic Cope <[email protected]>
@negz negz mentioned this pull request Jul 13, 2023
4 tasks
negz added 8 commits July 13, 2023 13:45
This commit introduces an updated design for Composition Functions.
Under this design Functions are long-running processes, like Providers.
Crossplane makes RunFunctionRequest RPCs to a Function using gRPC.

This updated design also sketches out an improved Function development
experience, as well as a few hypothetical 'generic' Functions. A generic
Function is a Function that works with any XR. Generic Functions extend
Crossplane with new ways to express Composition logic without actually
needing to develop a Function of your own.

Signed-off-by: Nic Cope <[email protected]>
Forgot to hit save in my editor. :D

Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
This reduces the amount of times 'name' appears and clarifies that the
Functions array is a pipeline of functions.

Signed-off-by: Nic Cope <[email protected]>
i.e. Be explicit that we want to asbtract away the fact that these are
servers.

Signed-off-by: Nic Cope <[email protected]>
negz added 7 commits July 13, 2023 13:45
This was mostly covered already, but now easier to find and with a touch
more detail (e.g. on dependencies).

Signed-off-by: Nic Cope <[email protected]>
This could be useful when running 'composition render'.

Signed-off-by: Nic Cope <[email protected]>
This was thrown in during the original design while we toyed with
Functions as being a way to do "day two stuff" (e.g. trigger backups). I
don't think anyone is confident that this is a good idea, so let's not
mention it for now.

Signed-off-by: Nic Cope <[email protected]>
Mixing up my CS terms. :)

Signed-off-by: Nic Cope <[email protected]>
This better frames it as what is is - input to a particular Function
call. The downside with calling if 'config' was that folks could mistake
it to mean it was configuring the Function _globally_, not just
configuring how one particular entry in one Composition's pipeline of
steps calls a Function.

Signed-off-by: Nic Cope <[email protected]>
// A Result of running a Function.
message Result {
// Severity of this result.
Severity severity = 1;
Copy link
Member

Choose a reason for hiding this comment

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

What is the expected behavior of Crossplane when a gRPC call returns an error? For example, should it trigger an event, modify the Sync condition etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we probably want it to fail the whole pipeline and return an error. Doing so would cause us to surface the error in a status condition and an event.

There's an entry in #3751 that mentions we should return errors with meaningful gRPC status codes, which will still be relevant under this design. In future we could potentially use this to better understand what errors are fatal vs returnable.

@negz
Copy link
Member Author

negz commented Jul 17, 2023

I believe we have good consensus that this is a better technical direction for Composition Functions. I'm going to merge this design so we can start moving in this direction in time for Crossplane v1.14.

I do want to call out again that the user and developer experience parts of this design should be considered examples of potential experiences. We need to test them with community members and will surely iterate on them between now, beta, and beyond.

What we do have confidence on is that it's better to build Functions as packages, and deploy them as long-running processes.

@gberche-orange
Copy link
Member

gberche-orange commented Jul 28, 2023

@negz late comment regarding generic functions and Starlak example:

Carvel ytt (https://carvel.dev/ytt/) is doing a great job of leveraging Starlak for specifically supporting a wide range of yaml files manipulation use-cases: mixing plain yaml and starlak fragments. I have successfully used ytt to craft existing compositions while leveraging full IDE support (see #3197 (comment)). I planned to test an alpha xfn using ytt cli (https://github.com/orange-cloudfoundry/paas-templates/issues/1813 ) before I learned about this new proposal.

Depending on roadmap for this proposal to be implemented, a containerized Function leveraging ytt cli might provide good feedback on how useful a ytt generic function could be, and help draft possible workflows such as "git-ops function authoring".

A ytt generic function might need to leverage

  • inline ytt fragments
  • ytt fragments loaded from configmap possibly mounted as volumes in the Deployment
  • ytt fragments loaded from a url/git repo

By "git-ops function authoring" workflow, I mean the practice of iteratively go through authoring/local-test/git push/deploy/trigger E2E test. I have fullfilled such workflow currently using ytt and kapp controller with 90 seconds cycle iterations (between push and start of kuttl test in the cluster).

It seems important to keeping in mind the requirement of short feedback loop during composition function authoring. I wonder how the containerized function workflow described at https://github.com/crossplane/crossplane/blob/master/design/design-doc-composition-functions.md#use-an-oci-container could accomodate "gitops workflows", with the following potential challenges

  • latency to build and push container images during authoring
  • available tooling to automate container image building and publication from popular gitops ecosystems (fluxcd, argocd, carvel) ?

See also related exchange into vfarcic/devops-toolkit-crossplane#4 (comment)

I switched to cdk8s for managing more complex (tedious) cases like Crossplane packages. The ability to download CRDs and generate libraries that, later on, I can use when constructing packages was a winner for me.

At that time @vfarcic opted for golang programming as a way to automate "tedious" crossplane authoring packages workflows.

@gberche-orange
Copy link
Member

gberche-orange commented Jul 29, 2023

Another late idea related to this proposal

a new pipeline array of Function calls

I was wondering whether there are other examples of such pattern in the kubernetes ecosystem.

The admission control system seems to share many characteristics with crossplane composition functions.

https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/

Was it considered to leverage the mutating webhook infrastructure to invoke composition functions developped in GPL or existing contemporary "Patch and Transform" libraries such as kyberno ?

This approach may have the potential to reuse and leverage many of the existing k8s infrastructure for invocating webhooks that crossplane would have to implement when issuing direct grpc calls to composition functions.

Examples benefits:

  • Reusable composition functions could be published as kyberno policies which can easily be discovered/shared
  • k8s webhook SDKs can be used to author GPL crossplane composition functions in many languages

I have not found traces of such discussion in the design proposal or from memory in the alpha composition design. Did I miss it ?

Ps: I'm currently out-of-office with limited connectivity. My responses on this thread will be late.

negz added a commit to negz/crossplane that referenced this pull request Aug 9, 2023
This one-pager proposes a lightweight implementation of the PRI
proposal, along with an alternative for ControllerConfig.

I believe this will be relevant for the in-flight Composition Functions
beta design, as well as for Providers.

crossplane#3601
crossplane#2671
crossplane#4306

Signed-off-by: Nic Cope <[email protected]>
rohit-rajput1 pushed a commit to rohit-rajput1/crossplane that referenced this pull request Sep 13, 2023
This one-pager proposes a lightweight implementation of the PRI
proposal, along with an alternative for ControllerConfig.

I believe this will be relevant for the in-flight Composition Functions
beta design, as well as for Providers.

crossplane#3601
crossplane#2671
crossplane#4306

Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Rohit31201 <[email protected]>
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.

7 participants