-
Notifications
You must be signed in to change notification settings - Fork 17
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
Promote composition functions to v1 #155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me from what I can tell. Looks like the implementation of serve serving both v1beta1 and v1 seems fine. I can't think of any crazy performance implications by running both.
v1 is currently identical to v1beta1. Upstream we have automation that makes sure the two protos are identical (apart from their package). Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
e3d5d39
to
b97d0d1
Compare
I tested this by:
The function successfully served both, so I think this is good to go. |
// A BetaServer is a v1beta1 FunctionRunnerServiceServer that wraps an identical | ||
// v1 FunctionRunnerServiceServer. This requires the v1 and v1beta1 protos to be | ||
// identical. | ||
// | ||
// Functions were promoted from v1beta1 to v1 in Crossplane v1.17. Crossplane | ||
// v1.16 and earlier only sends v1beta1 RunFunctionRequests. Functions should | ||
// use the BetaServer for backward compatibility, to support Crossplane v1.16 | ||
// and earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most interesting part of this PR. Everything else is just:
- Replicating the v1 and v1beta1 protos from Crossplane
s/v1beta1/v1/
for all the imports of the protos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks reasonable here to me, but I do have a couple questions about some parts of it work. Thank you @negz!
} | ||
|
||
// A RunFunctionRequest requests that the Composition Function be run. | ||
message RunFunctionRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remind me how this proto file content gets here to the SDK in the first place? is that automated, or is it a manual operation to duplicate it from crossplane/crossplane? do we have mechanisms in place to keep it in sync over time? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just manual currently. It'd be nice to automate, but I'm not sure how to do that. Something like Renovate that noticed when it was updated in c/c and copied it over would be ideal.
I was accidentally always returning an empty response. This fixes that, and adds a unit test. Signed-off-by: Nic Cope <[email protected]>
Tested again. Here's the output to make sure I didn't mix it up again: This is a v1 request and response: # $ ~/control/crossplane/crossplane/_output/bin/linux_arm64/crank render xr.yaml composition.yaml functions.yaml
---
apiVersion: example.crossplane.io/v1
kind: XR
metadata:
name: example-xr
status:
conditions:
- lastTransitionTime: "2024-01-01T00:00:00Z"
message: 'Unready resources: bucket'
reason: Creating
status: "False"
type: Ready
---
apiVersion: s3.aws.upbound.io/v1beta1
kind: Bucket
metadata:
annotations:
crossplane.io/composition-resource-name: bucket
generateName: example-xr-
labels:
crossplane.io/composite: example-xr
ownerReferences:
- apiVersion: example.crossplane.io/v1
blockOwnerDeletion: true
controller: true
kind: XR
name: example-xr
uid: ""
spec:
forProvider:
region: us-east-2 This is a v1beta1 request and response, from the same function: # $ ~/control/crossplane/crossplane/_output/bin/linux_arm64/crank beta render xr.yaml composition.yaml functions.yaml
---
apiVersion: example.crossplane.io/v1
kind: XR
metadata:
name: example-xr
status:
conditions:
- lastTransitionTime: "2024-01-01T00:00:00Z"
message: 'Unready resources: bucket'
reason: Creating
status: "False"
type: Ready
---
apiVersion: s3.aws.upbound.io/v1beta1
kind: Bucket
metadata:
annotations:
crossplane.io/composition-resource-name: bucket
generateName: example-xr-
labels:
crossplane.io/composite: example-xr
ownerReferences:
- apiVersion: example.crossplane.io/v1
blockOwnerDeletion: true
controller: true
kind: XR
name: example-xr
uid: ""
spec:
forProvider:
region: us-east-2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for fixing the beta response issue and adding a test! now it all looks good to me! 😎
Description of your changes
This PR compliments crossplane/crossplane#5885, which promotes functions to v1 in Crossplane.
Crossplane v1.17 and above will send requests to
apiextensions.fn.proto.v1.FunctionRunnerService
. It'll fall back toapiextensions.fn.proto.v1beta1.FunctionRunnerService
if v1 isn't available. Crossplane v1.16 and earlier will only send requests toapiextensions.fn.proto.v1beta1.FunctionRunnerService
.Going forward we want functions to serve
apiextensions.fn.proto.v1.FunctionRunnerService
. However, to avoid breaking compatibility with Crossplane v1.16 and earlier they'll also need to serveapiextensions.fn.proto.v1beta1.FunctionRunnerService
.This PR:
BetaServer
middleware that serves v1beta1 requests by wrapping a v1 server.This wrapping approach only works because we intend v1 to be identical to v1beta1. That is, we promoted v1beta1 to v1 without making any breaking changes. In upstream Crossplane the v1beta1 proto is now automatically replicated from the v1 proto to ensure they remain identical.
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested