feat: extra resources

[phisco]

Description of your changes

Updates the protobuf schema and adds a few helpers functions for crossplane/crossplane#5099.

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

[negz]

A few nits, but mostly this LGTM.

[negz]

I think it's implementation time. Do we want a oneOf? 😉

[phisco]

I'd love to, but it would look like this:

message ResourceSelector {
  string api_version = 1;
  string kind = 2;

  oneof match {
    string name = 3;
    MatchLabels labels = 4;
  }
}

message MatchLabels {
  map<string, string> ??? = 1;
}

Unfortunately a map can't be part of a oneof and given that there are no type aliases, we would need to define another message with another field inside which I can't think of a good name for 😭

[negz]

Maybe:

message ResourceSelector {
  string api_version = 1;
  string kind = 2;

  oneof match {
    string match_name = 3;
    MatchLabels match_labels = 4;
  }
}

message MatchLabels {
  map<string, string> labels = 1;
}

I still don't love it though. I could go either way - probably treating it as a oneof by convention would be fine.

Or I guess we could allow a name and labels and just handle it as "I want a resource with this name and these labels", even though that would be unlikely to actually be useful for anyone.

[phisco]

that would mean the generated Go struct would need to be accessed as ResourceSelector.Match.MatchLabels.Labels 😞

[phisco]

another option could be:

enum ResourceSelectorType {
  RESOURCE_SELECTOR_TYPE_UNSPECIFIED = 0;

  // Match resources by name.
  MATCH_NAME = 1;

  // Match resources by label.
  MATCH_LABELS = 2;
}

// ResourceSelector selects a group of resources, either by name or by label.
message ResourceSelector {
  string api_version = 1;
  string kind = 2;

  // Type of selector.
  ResourceSelectorType type = 3;

  optional string match_name = 4;
  map<string, string> match_labels = 5;
}

not entirely sure it's idiomatic to have the type in protobuf 🤔

[negz]

that would mean the generated Go struct would need to be accessed as ResourceSelector.Match.MatchLabels.Labels

If I follow https://protobuf.dev/reference/go/go-generated/#oneof correctly I think you get generated getters that allow this:

rs := &ResourceSelector{}
l := rs.GetMatchLabels().GetLabels()

Still not amazing, but maybe the best we can do?

not entirely sure it's idiomatic to have the type in protobuf

I feel like we should use oneOf - it seems like the idiomatic thing to do despite the stuttering and fairly ugly generated Go code.

[phisco]

These would be the changes needed on crossplane/crossplane#5247.
As you can see, the getters would not be so painful indeed, but the struct would be really error-prone in non-strongly typed languages, it was already hard to get it right even in Go.
I'd rather go with a type field and add some validation using https://github.com/bufbuild/protovalidate.

diff

diff --git a/apis/apiextensions/fn/proto/v1beta1/run_function.proto b/apis/apiextensions/fn/proto/v1beta1/run_function.proto
index c274d21c..171916ff 100644
--- a/apis/apiextensions/fn/proto/v1beta1/run_function.proto
+++ b/apis/apiextensions/fn/proto/v1beta1/run_function.proto
@@ -124,8 +124,14 @@ message ResourceSelector {
   string api_version = 1;
   string kind = 2;
 
-  optional string match_name = 3;
-  map<string, string> match_labels = 4;
+  oneof match {
+    string match_name = 3;
+    MatchLabels match_labels = 4;
+  }
+}
+
+message MatchLabels {
+  map<string, string> labels = 1;
 }
 
 // ResponseMeta contains metadata pertaining to a RunFunctionResponse.
diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go
index 085242ca..afc0d6ae 100644
--- a/internal/controller/apiextensions/composite/composition_functions.go
+++ b/internal/controller/apiextensions/composite/composition_functions.go
@@ -535,7 +535,7 @@ func (e *ExistingExtraResourcesGetter) Get(ctx context.Context, selector *v1beta
 	list.SetAPIVersion(selector.GetApiVersion())
 	list.SetKind(selector.GetKind())
 
-	if err := e.client.List(ctx, list, client.MatchingLabels(selector.GetMatchLabels())); err != nil {
+	if err := e.client.List(ctx, list, client.MatchingLabels(selector.GetMatchLabels().GetLabels())); err != nil {
 		return nil, errors.Wrap(err, errListExtraResources)
 	}
 
diff --git a/internal/controller/apiextensions/composite/composition_functions_test.go b/internal/controller/apiextensions/composite/composition_functions_test.go
index ae41ea24..5a95c2bc 100644
--- a/internal/controller/apiextensions/composite/composition_functions_test.go
+++ b/internal/controller/apiextensions/composite/composition_functions_test.go
@@ -658,12 +658,16 @@ func TestFunctionCompose(t *testing.T) {
 								"existing": {
 									ApiVersion: "test.crossplane.io/v1",
 									Kind:       "Foo",
-									MatchName:  ptr.To("existing"),
+									Match: &v1beta1.ResourceSelector_MatchName{
+										MatchName: "existing",
+									},
 								},
 								"missing": {
 									ApiVersion: "test.crossplane.io/v1",
 									Kind:       "Bar",
-									MatchName:  ptr.To("missing"),
+									Match: &v1beta1.ResourceSelector_MatchName{
+										MatchName: "missing",
+									},
 								},
 							},
 						}
@@ -1422,7 +1426,9 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
-					MatchName:  ptr.To("cool-resource"),
+					Match: &v1beta1.ResourceSelector_MatchName{
+						MatchName: "cool-resource",
+					},
 				},
 				c: &test.MockClient{
 					MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
@@ -1453,8 +1459,12 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
-					MatchLabels: map[string]string{
-						"cool": "resource",
+					Match: &v1beta1.ResourceSelector_MatchLabels{
+						MatchLabels: &v1beta1.MatchLabels{
+							Labels: map[string]string{
+								"cool": "resource",
+							},
+						},
 					},
 				},
 				c: &test.MockClient{
@@ -1526,7 +1536,9 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
-					MatchName:  ptr.To("cool-resource"),
+					Match: &v1beta1.ResourceSelector_MatchName{
+						MatchName: "cool-resource",
+					},
 				},
 				c: &test.MockClient{
 					MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{Resource: "Foo"}, "cool-resource")),
@@ -1544,7 +1556,9 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
-					MatchName:  ptr.To("cool-resource"),
+					Match: &v1beta1.ResourceSelector_MatchName{
+						MatchName: "cool-resource",
+					},
 				},
 				c: &test.MockClient{
 					MockGet: test.NewMockGetFn(errBoom),
@@ -1561,8 +1575,12 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
-					MatchLabels: map[string]string{
-						"cool": "resource",
+					Match: &v1beta1.ResourceSelector_MatchLabels{
+						MatchLabels: &v1beta1.MatchLabels{
+							Labels: map[string]string{
+								"cool": "resource",
+							},
+						},
 					},
 				},
 				c: &test.MockClient{

[phisco]

The type option would look like this:

diff

diff --git a/apis/apiextensions/fn/proto/v1beta1/run_function.proto b/apis/apiextensions/fn/proto/v1beta1/run_function.proto
index c274d21c..f3b77193 100644
--- a/apis/apiextensions/fn/proto/v1beta1/run_function.proto
+++ b/apis/apiextensions/fn/proto/v1beta1/run_function.proto
@@ -124,8 +124,18 @@ message ResourceSelector {
   string api_version = 1;
   string kind = 2;
 
-  optional string match_name = 3;
-  map<string, string> match_labels = 4;
+  enum Type {
+    // Select resources by name.
+    MATCH_NAME = 0;
+
+    // Select resources by label.
+    MATCH_LABELS = 1;
+  }
+
+  Type type = 3;
+
+  optional string match_name = 4;
+  map<string, string> match_labels = 5;
 }
 
 // ResponseMeta contains metadata pertaining to a RunFunctionResponse.
diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go
index 085242ca..a058527e 100644
--- a/internal/controller/apiextensions/composite/composition_functions.go
+++ b/internal/controller/apiextensions/composite/composition_functions.go
@@ -504,11 +504,12 @@ func NewExistingExtraResourcesGetter(c client.Reader) *ExistingExtraResourcesGet
 }
 
 // Get fetches resources requested by functions using the provided client.Reader.
-func (e *ExistingExtraResourcesGetter) Get(ctx context.Context, selector *v1beta1.ResourceSelector) (*v1beta1.Resources, error) {
+func (e *ExistingExtraResourcesGetter) Get(ctx context.Context, selector *v1beta1.ResourceSelector) (*v1beta1.Resources, error) { 
 	if selector == nil {
 		return nil, errors.New(errNilResourceSelector)
 	}
-	if selector.GetMatchName() != "" {
+	switch t := selector.GetType(); t {
+	case v1beta1.ResourceSelector_MATCH_NAME:
 		// Fetch a single resource.
 		r := &kunstructured.Unstructured{}
 		r.SetAPIVersion(selector.GetApiVersion())
@@ -528,28 +529,31 @@ func (e *ExistingExtraResourcesGetter) Get(ctx context.Context, selector *v1beta
 			return nil, errors.Wrap(err, errExtraResourceAsStruct)
 		}
 		return &v1beta1.Resources{Items: []*v1beta1.Resource{{Resource: o}}}, nil
-	}
 
-	// Fetch a list of resources.
-	list := &kunstructured.UnstructuredList{}
-	list.SetAPIVersion(selector.GetApiVersion())
-	list.SetKind(selector.GetKind())
+	case v1beta1.ResourceSelector_MATCH_LABELS:
+		// Fetch a list of resources.
+		list := &kunstructured.UnstructuredList{}
+		list.SetAPIVersion(selector.GetApiVersion())
+		list.SetKind(selector.GetKind())
 
-	if err := e.client.List(ctx, list, client.MatchingLabels(selector.GetMatchLabels())); err != nil {
-		return nil, errors.Wrap(err, errListExtraResources)
-	}
+		if err := e.client.List(ctx, list, client.MatchingLabels(selector.GetMatchLabels())); err != nil {
+			return nil, errors.Wrap(err, errListExtraResources)
+		}
 
-	resources := make([]*v1beta1.Resource, len(list.Items))
-	for i, r := range list.Items {
-		r := r
-		o, err := AsStruct(&r)
-		if err != nil {
-			return nil, errors.Wrap(err, errExtraResourceAsStruct)
+		resources := make([]*v1beta1.Resource, len(list.Items))
+		for i, r := range list.Items {
+			r := r
+			o, err := AsStruct(&r)
+			if err != nil {
+				return nil, errors.Wrap(err, errExtraResourceAsStruct)
+			}
+			resources[i] = &v1beta1.Resource{Resource: o}
 		}
-		resources[i] = &v1beta1.Resource{Resource: o}
-	}
 
-	return &v1beta1.Resources{Items: resources}, nil
+		return &v1beta1.Resources{Items: resources}, nil
+	default:
+		return nil, errors.Errorf("unsupported resource selector type %q", t)
+	}
 }
 
 // An ExistingComposedResourceObserver uses an XR's resource references to load
diff --git a/internal/controller/apiextensions/composite/composition_functions_test.go b/internal/controller/apiextensions/composite/composition_functions_test.go
index ae41ea24..932d4907 100644
--- a/internal/controller/apiextensions/composite/composition_functions_test.go
+++ b/internal/controller/apiextensions/composite/composition_functions_test.go
@@ -1422,6 +1422,7 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
+					Type:       v1beta1.ResourceSelector_MATCH_NAME,
 					MatchName:  ptr.To("cool-resource"),
 				},
 				c: &test.MockClient{
@@ -1453,6 +1454,7 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
+					Type:       v1beta1.ResourceSelector_MATCH_LABELS,
 					MatchLabels: map[string]string{
 						"cool": "resource",
 					},
@@ -1526,6 +1528,7 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
+					Type:       v1beta1.ResourceSelector_MATCH_NAME,
 					MatchName:  ptr.To("cool-resource"),
 				},
 				c: &test.MockClient{
@@ -1544,6 +1547,7 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
+					Type:       v1beta1.ResourceSelector_MATCH_NAME,
 					MatchName:  ptr.To("cool-resource"),
 				},
 				c: &test.MockClient{
@@ -1561,6 +1565,7 @@ func TestExistingExtraResourcesGetterGet(t *testing.T) {
 				selector: &v1beta1.ResourceSelector{
 					ApiVersion: "test.crossplane.io/v1",
 					Kind:       "Foo",
+					Type:       v1beta1.ResourceSelector_MATCH_LABELS,
 					MatchLabels: map[string]string{
 						"cool": "resource",
 					},

[negz]

The oneOf diff looks simpler to me. 🤔

My vote is oneOf - it has its downsides but my gut is that we should do the idiomatic/least surprising thing and use the protobuf feature designed for this case.

FWIW if I'm following https://protobuf.dev/reference/python/python-generated/#oneof correctly in Python it would be something like:

rsp.requirements["some-resources"].api_version = "example.org/v1"
rsp.requirements["some-resources"].api_version = "Friend"
rsp.requirements["some-resources"].match_labels.labels = {"example.org/match": "true"}

[phisco]

I'm not sure the type would be less idiomatic, but it's fine with me, I'll apply that tomorrow 👍