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

Add a design document for 'Composition Functions' #2886

Merged
merged 14 commits into from Nov 17, 2022
Merged

Conversation

negz
Copy link
Member

@negz negz commented Feb 9, 2022

Description of your changes

Fixes #3001
Fixes #2959
Fixes #2958

This commit adds a design document for 'Composition Functions', which
we've previously referred to as 'Custom Compositions'. See #2524 for the
full context.

I have:

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

How has this code been tested

@sergenyalcin and I have built a few proof of concepts, notably #3001
and #2959 to inform this design.

@negz
Copy link
Member Author

negz commented Mar 24, 2022

We're currently prototyping two approaches to actually implement this.

  1. Prototype rootless container based composition functions #3001
  2. Check advantages/disadvantages using the Pod - Job - CronJob to run KRM functions #2959

The second approach also involves #2958. Each approach has tradeoffs and we may pick either one or allow a choice.

The first approach would likely result in

  • Pro: Faster reconcile times - no need to schedule and run a pod.
  • Pro: No need for a wrapper process - the controller could pass stdin/stdout etc between functions.
  • Pro: Likely a better integration with web hook based functions (a function of the two above pros).
  • Con: Noisy neighbors (multiple function processes in the same 'runner' container)
  • Con: No isolation (network/process, etc wise) between function processes in the same container.
  • Con: "Reinventing the wheel" (pulling, caching function images)

The second approach is roughly the inverse:

  • Pro: Leverages existing Kubernetes infra (Pods) - caching, reruns, isolation, etc.
  • Con: No native ordering of containers within a Pod (needs a wrapper process ala Argo Emissary).
  • Con: Likely scheduling issues delays - need to create, schedule, and run a pod for each reconcile loop iteration.

@negz negz force-pushed the cc branch 2 times, most recently from dd79c11 to fc87ef3 Compare April 23, 2022 01:23
@negz negz changed the title Add a design document for 'Custom Composition' Add a design document for 'Composition Functions' Apr 23, 2022
@negz negz force-pushed the cc branch 5 times, most recently from fb3a7e2 to 68f3b68 Compare April 23, 2022 02:14
@negz negz marked this pull request as ready for review April 23, 2022 02:14
@negz negz removed request for muvaf and hasheddan April 23, 2022 02:17
@negz
Copy link
Member Author

negz commented Apr 23, 2022

This is now ready for review. I'd like to get approvals from @sergenyalcin and @turkenh - perhaps @sergenyalcin could do a first pass since he has the most context on this design.

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.

Excited to see this coming 🔥

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

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

Thanks for the review folks!

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 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
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
design/design-doc-composition-functions.md Outdated Show resolved Hide resolved
@negz negz mentioned this pull request Apr 29, 2022
3 tasks
@negz
Copy link
Member Author

negz commented Apr 30, 2022

I noticed while testing #3062 that the default seccomp profile in Docker and containerd reject the unshare syscall that we need to make rootless containers. Kubernetes runs with the Unconstrained seccomp profile by default so this shouldn't be a problem for most folks, but that may change with kubernetes/kubernetes#101943.

I'm not too worried about this - I believe that even if RuntimeDefault becomes the default seccomp profile in Kubernetes we'd only be asking folks to give us two syscalls that aren't included by default - unshare and mount.

@salaboy
Copy link

salaboy commented Aug 3, 2022

@negz @st3v @turkenh folks.. I hope my comments here are not a problem. I am hoping to be able to help with this as I am quite passionate about functions. As I've mentioned in one of my comments, as a user I believe that the metacontroller approach is the way to go: https://metacontroller.github.io/metacontroller/guide/create.html, and I will be more than happy to build a PoC around that with Crossplane.. because at least in my head adding function references to compositions to run custom logic shouldn't be that complicated :)

@negz
Copy link
Member Author

negz commented Aug 23, 2022

@salaboy Thanks for your comments. I don't immediately share your concerns - my feeling is that what's proposed here should make it easier for folks to build and deploy functions vs requiring them to do so as separate long running processes. Consider especially:

  • Folks who are not Kubernetes experts
  • Configuring secure HTTP based authentication between Crossplane and functions.
  • Providing SDKs to make writing functions easier.
  • Functions that leverage / are light wrappers on other tools (e.g. Helm) rather than complete bespoke logic.

I'm just getting back around to working on this - happy to chat with you about it some time.

negz and others added 12 commits October 7, 2022 18:06
Fixes crossplane#3001
Fixes crossplane#2959
Fixes crossplane#2958

This commit adds a design document for 'Composition Functions', which
we've previously referred to as 'Custom Compositions'. See crossplane#2524 for the
full context.

This is the
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
As well as a new `type: Chroot` alternative that could be used if
compatibility becomes an issue.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Including timeouts, network isolation, and resource limits.

Signed-off-by: Nic Cope <nicc@rk0n.org>
This is just a fresh take on what was already there after a few months
of discussion and thought.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
The design of the ResourceList type was heavily inspired by KRM
functions, but the implementation proposed by the Composition Functions
design was already incompatible (see "Using KRM Spec Compatible
Functions" under "Alternatives Considered"). It also recently occurred
to me that Crossplane may need information that isn't easily modelled as
part of a simple array of Kubernetes resources - for example it may need
to know how to derive an XR's connection details from its composed
resources'.

To this end it seemed better to break away completely from ResourceList
and design a similar type that was purpose built for Composition
Functions. Some improvements in this type include:

* Better schema for `composite` - we can ensure exactly one is required,
  and easily find it without iterating through the list of resources.
* We can use real (required) fields in the resources array, not special
  annotations, for things like the name of the array entry.
* We can use fields in the resources array to return connection details
  (or instructions on how to derive connection details)

Signed-off-by: Nic Cope <nicc@rk0n.org>
This approach is a nice compromise between webhooks and 'built-in'
support for running containers.

Containerized functions have less operational overhead than a webhook
function - they allow the function author to simply write and push the
function rather than needing to deploy, maintain, and secure it. The
downside of Containerized functions is that some other system must do
all of these things.

Previously this document proposed that Crossplane be that system - that
it would run Composition Functions itself. Given that there are many
different ways to run containers, each with their own tradeoffs,
breaking out the function runner into a separate implementation offers
extra flexibility.

Signed-off-by: Nic Cope <nicc@rk0n.org>
This could allow us to specify extra runner config (e.g. credentials) in
future, if required.

Signed-off-by: Nic Cope <nicc@rk0n.org>
GitHub doesn't recognise the latter.

Signed-off-by: Nic Cope <nicc@rk0n.org>
@negz
Copy link
Member Author

negz commented Oct 10, 2022

https://gist.github.com/negz/c5a5b4f82b06882906b08af377a609a4

Drawing attention to the above gist. I used this to sketch out the FunctionIO API, and it sparked a good discussion between @turkenh and I in the comments.

crossplane#2886 (comment)

The above comment thread contains the discussion that lead to this
change. The primary motiviation for this change is that sometimes we'll
want to provide functions with context (observed state) that they can't
mutate, and we want to clearly distinguish that from state that they can
mutate.

A simple example is the status of composed resources - functions may use
this status as an input, but can't mutate it. Connection details are a
more complex example, in that the format used to specify desired
connection details differs from the format used to surface existing,
observed connection details.

This approach has some additional benefits:

* All functions are able to know the observed state of the world, which
  may differ from the accumulated desired state.
* Desired state may be returned in a more succinct 'overlay' style. This
  may be more or less onerous than mutating observed state depending on
  the scenario.
* Passing observed state (without accidentally mutating it) is likely to
  be less error prone.

Signed-off-by: Nic Cope <nicc@rk0n.org>
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.

Thank you @negz! Really excited to see this coming 🚀

@negz negz merged commit 529d234 into crossplane:master Nov 17, 2022
@negz negz deleted the cc branch November 17, 2022 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants