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

Improve support of typed objects #93

Open
Kidswiss opened this issue Nov 21, 2023 · 5 comments · May be fixed by #150
Open

Improve support of typed objects #93

Kidswiss opened this issue Nov 21, 2023 · 5 comments · May be fixed by #150
Labels
enhancement New feature or request

Comments

@Kidswiss
Copy link

Kidswiss commented Nov 21, 2023

What problem are you facing?

We've been using the composition functions basically since day one. When starting with them, we had lengthy discussions how to handle the objects.

We decided to use and import the already available go structs that most providers have (provider-kubernetes, provider-helm, etc.). This makes working with the objects a lot more comfortable, as the compiler will help with type checking.

While there's a composed.From(rutnime.Object) function there's no function for the other way round e.g. composed.To() witch could marshal the composed resources directly into a typed object.

Additionally, it might make sense to change composed.From(rutnime.Object) to composed.From(resource.Managed) to avoid adding a plain k8s object to the desired state. We've had the issues before that someone added plain k8s objects without wrapping them in a objects.kubernetes.crossplane.io first. Which can lead to very annoying to debug bugs.

Also, it might be worth adding functions to response to directly add objects to the desired map.

For example:

response.AddDesiredComposedResource(resp RunFuntionResponse, o resource.Managed, name string)

Which would do the marshalling and add (or override) the given object in the desired composed objects map.

IMHO these changes would greatly improve the developer experience of the SDK. It also helps to avoid errors like here: #68

I've already implemented such typed functions in our framework: https://github.com/vshn/appcat/blob/change/xplane114/pkg/comp-functions/runtime/function_mgr.go#L282
And it's working great so far, it's already a lot better than the previous runtime.RawExtension in the FunctionIO.

How could Crossplane help solve your problem?

It already does, these are just suggestions to make it even better. :)

@Kidswiss Kidswiss added the enhancement New feature or request label Nov 21, 2023
@negz
Copy link
Member

negz commented Jan 4, 2024

I'm definitely happy to have any contributions that make working with structured objects easier. We just have to be sure that it's still possible (and simple) to work with unstructured objects too.

Additionally, it might make sense to change composed.From(rutnime.Object) to composed.From(resource.Managed) to avoid adding a plain k8s object to the desired state.

Keep in mind you can compose other Crossplane types, like XRs and ProviderConfigs. resource.Managed would be too limiting.

@BigGold1310
Copy link
Contributor

I propose a method to simplify working with structured objects in Crossplane functions. Currently, there are two primary use-cases for functions: one that operates with unstructured data and another that benefits significantly from structured objects. The Crossplane documentation acknowledges both use-cases:

Some people design composition functions for you to use them with any kind of composite resource. Function Patch and Transform and Function Auto Ready work with any kind of composite resource.
Another common pattern is to write a composition function specific to one kind of composite resource. The function contains all the logic needed to tell Crossplane what resources to create when you create a composite resource.

I've explored numerous open-source functions and am now working on implementing a function specific to a particular kind of composite resource. During this process, I found that the current SDK and function template are overly generic, primarily focused on broadly applicable function implementations. Additionally, there is a lack of documentation on generating the XRD from a Go structure to use it as a typed object.

To address these issues, I propose creating a manager/handler similar to the controller-runtime manager/reconciler. This manager/handler would register potential handlers (dedicated logic units for each logical function or topic of concern) within the function. Furthermore, I envision including helper methods to efficiently get/set the observed/desired state of objects. These methods would be akin to the current From/To functions but would incorporate a query parameter similar to the Kubernetes client.

If this approach is considered viable, I would be eager to start developing a prototype.

@jbw976
Copy link
Member

jbw976 commented Jul 1, 2024

Thanks for taking the time to write up this idea @BigGold1310. Would you be able to add a couple examples that demonstrate more tangibly what you envision? e.g.,:

  • what would some of these helper methods look like (signature and dev experience)?
  • what would the potential handlers look like?

@BigGold1310
Copy link
Contributor

@jbw976 Sure, it took me some time to complete the first iteration. I'm eager for your feedback. I hope this meets your expectations.

Manager / Function

The function initializes an optional manager responsible for orchestrating handlers. It should also take care creating the response based on the returned Handler errors.

// RunFunction runs the Function.
func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRequest) (*fnv1beta1.RunFunctionResponse, error) {
	var err error
	f.log.Info("Running function", "tag", req.GetMeta().GetTag())


	manager := runtime.NewManager(ctx, req)
	if err = manager.RegisterHandler(AWSK8sHandler, &K8sXRD{});  err != nil {
		f.log.Error(err, "unable to register handler", "handler", "AWSK8s")
	}
	if err = manager.RegisterHandler(GCPK8sHandler, &K8sXRD{});  err != nil {
		f.log.Error(err, "unable to register handler", "handler", "GCPK8s")
	}

	rsp, err := manager.GetResponse()
	if err != nil {
		response.Fatal(rsp, errors.Wrapf(err, "can't create response %T", req))
		return rsp, nil
	}

	return rsp, nil
}

Potential Handler & Helper

Signature

func MyHandler(ctx, client) error

Helper

My idea here is to create a client that simplifies the process of getting, setting, and updating objects.

client.GetComposite(&type)
client.SetComposite(&type)
client.GetInput(&type)
client.GetObserverd(string, &type)
client.GetDesired(string, &type)
client.SetDesired(string, &type)
// We could also provide raw access
client.req // Request
client.rsp // Response

Example method

// AWSK8sHandler handles the creation of K8s clusters
func AWSK8sHandler(ctx, client runtime.Client) error {
	xrd := &K8sXRD{}
	client.GetComposite(xrd)
	client.SetComposite(xrd)
	obj := &EKS{}
	client.GetObserverd("name", obj)
	client.GetDesired("name", obj)
	client.SetDesired("name", obj)
        log.Debug("Handled AWSK8sHandler: %T\n", xrd)
}

Potential ideas:

  • Allow registering a handler only for specific XRDs instead of all of them. If so, it might be better to use a dedicated method like .For() instead of the current RegisterHandler(function, type).
  • Implement a setup method that can evaluate conditions based on the XRD values to determine if a handler should be registered or run.
  • Create objects with a Handle method. While registering the object, a client instance can be passed/added to it.

@BigGold1310 BigGold1310 linked a pull request Jul 8, 2024 that will close this issue
2 tasks
@jbw976
Copy link
Member

jbw976 commented Jul 25, 2024

Cool @BigGold1310, thanks for taking the time to show the devEx for your ideas a bit further. A few quick thoughts:

  • A typed client that provides a strongly typed experience for dealing with resources does sound like a nice idea with better ergonomics. An experience similar to the controller-runtime Client, e.g. that can do a Get operation on a typed object:
  • I'm not sure I understand the Manager/Handlers as well as the Client idea just yet though
    • I thought it the Manager would route route incoming RunFunctionRequest to handlers depending on the type of the XR, but from your example it looks like it's routing to different runtime implementations (AWS, GCP, etc.) for one given XR type.
      • How does the manager know which handler to route to?
      • What's the main use case for this pattern again? Is an entire Manager necessary instead of a more straightforward switch statement that branches to different functions?
    • What's the expectation of the handlers duties? Does it perform all the operations you'd expect from the main RunFunction body, including setting the desired resources, but excluding returning a response? Trying to glean that from your example, but more detail could be helpful
    • Build an entire platform in your language of choice crossplane#5172 would definitely be useful here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants