Repository: crossplane/crossplane-runtime
Pull request #153 (2020-04-17 03:21:28)

Additional reference selection and resolution helpers
<!--
Thank you for helping to improve Crossplane!

We strongly recommend you look through our contributor guide at https://git.io/fj2m9
if this is your first time opening a Crossplane pull request. You can find us in
https://slack.crossplane.io/messages/dev if you need any help contributing.
-->

### Description of your changes
<!--
Briefly describe what this pull request does. Be sure to direct your reviewers'
attention to anything that needs special consideration.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

Fixes #
-->
A small handful of our existing reference resolvers support resolving an array of references to an array of values. This PR adds a convenience method to handle this case (alongside a few trivial updates to `pkg/reference` in separate commits). I opted to closely match the existing `Resolve` method's logic in this new implementation, specifically:

* The selector is only considered if no references have been set.
* The references are only considered if no values have been set.

This means, like `Resolve`, `ResolveMultiple` will only resolve references once. The CR author must clear any existing values to trigger re-resolution of references, and must clear any values _and_ references to trigger re-selection of references.


### Checklist
<!--
Please run through the below readiness checklist. The first two items are
relevant to every Crossplane pull request.
-->
I have:
- [x] Run `make reviewable` to ensure this PR is ready for review.
- [x] Ensured this PR contains a neat, self documenting set of commits.
- [ ] Updated any relevant [documentation], [examples], or [release notes].
- [ ] Updated the RBAC permissions in [`clusterrole.yaml`] to include any new types.

[documentation]: https://github.com/crossplane/crossplane/tree/master/docs
[examples]: https://github.com/crossplane/crossplane/tree/master/cluster/examples
[release notes]: https://github.com/crossplane/crossplane/tree/master/PendingReleaseNotes.md
[`clusterrole.yaml`]: https://github.com/crossplane/crossplane/blob/master/cluster/charts/crossplane/templates/clusterrole.yaml


Patch:
diff --git a/pkg/reference/reference.go b/pkg/reference/reference.go
index 9e46128..9fc21df 100644
--- a/pkg/reference/reference.go
+++ b/pkg/reference/reference.go
@@ -36,6 +36,26 @@ const (
 	errNoValue     = "referenced field was empty (reference may not yet be ready)"
 )
 
+// NOTE(negz): There are many equivalents of FromPtrValue and ToPtrValue
+// throughout the Crossplane codebase. We duplicate them here to reduce the
+// number of packages our API types have to import to support references.
+
+// FromPtrValue adapts a string pointer field for use as a CurrentValue.
+func FromPtrValue(v *string) string {
+	if v == nil {
+		return ""
+	}
+	return *v
+}
+
+// ToPtrValue adapts a ResolvedValue for use as a string pointer field.
+func ToPtrValue(v string) *string {
+	if v == "" {
+		return nil
+	}
+	return &v
+}
+
 // To indicates the kind of managed resource a reference is to.
 type To struct {
 	Managed resource.Managed
@@ -95,11 +115,54 @@ func (rr ResolutionResponse) Validate() error {
 	return nil
 }
 
-// A Resolver selects and resolves references to managed resources.
-type Resolver interface {
-	// Resolve the supplied ResolutionRequest. The returned ResolutionResponse
-	// always contains valid values unless an error was returned.
-	Resolve(ctx context.Context, req ResolutionRequest) (ResolutionResponse, error)
+// A MultiResolutionRequest requests that several references to a particular
+// kind of managed resource be resolved.
+type MultiResolutionRequest struct {
+	CurrentValues []string
+	References    []v1alpha1.Reference
+	Selector      *v1alpha1.Selector
+	To            To
+	Extract       ExtractValueFn
+}
+
+// IsNoOp returns true if the supplied MultiResolutionRequest cannot or should
+// not be processed.
+func (rr MultiResolutionRequest) IsNoOp() bool {
+	// We don't resolve values that are already set; we effectively cache
+	// resolved values. The CR author can invalidate the cache and trigger a new
+	// resolution by explicitly clearing the resolved values. This is a little
+	// unintuitive for the APIMultiResolver but mimics the UX of the APIResolver
+	// and simplifies the overall mental model.
+	if len(rr.CurrentValues) > 0 {
+		return true
+	}
+
+	// We can't resolve anything if neither a reference nor a selector were
+	// provided.
+	return len(rr.References) == 0 && rr.Selector == nil
+}
+
+// A MultiResolutionResponse returns the result of several reference
+// resolutions. The returned values are always safe to set if resolution was
+// successful.
+type MultiResolutionResponse struct {
+	ResolvedValues     []string
+	ResolvedReferences []v1alpha1.Reference
+}
+
+// Validate this MultiResolutionResponse.
+func (rr MultiResolutionResponse) Validate() error {
+	if len(rr.ResolvedValues) == 0 {
+		return errors.New(errNoMatches)
+	}
+
+	for _, v := range rr.ResolvedValues {
+		if v == "" {
+			return errors.New(errNoValue)
+		}
+	}
+
+	return nil
 }
 
 // An APIResolver selects and resolves references to managed resources in the
@@ -152,6 +215,50 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol
 	return ResolutionResponse{}, errors.New(errNoMatches)
 }
 
+// ResolveMultiple resolves the supplied MultiResolutionRequest. The returned
+// MultiResolutionResponse always contains valid values unless an error was
+// returned.
+func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRequest) (MultiResolutionResponse, error) {
+	// Return early if from is being deleted, or the request is a no-op.
+	if meta.WasDeleted(r.from) || req.IsNoOp() {
+		return MultiResolutionResponse{ResolvedValues: req.CurrentValues, ResolvedReferences: req.References}, nil
+	}
+
+	// The references are already set - resolve them.
+	if len(req.References) > 0 {
+		vals := make([]string, len(req.References))
+		for i := range req.References {
+			if err := r.client.Get(ctx, types.NamespacedName{Name: req.References[i].Name}, req.To.Managed); err != nil {
+				return MultiResolutionResponse{}, errors.Wrap(err, errGetManaged)
+			}
+			vals[i] = req.Extract(req.To.Managed)
+		}
+
+		rsp := MultiResolutionResponse{ResolvedValues: vals, ResolvedReferences: req.References}
+		return rsp, rsp.Validate()
+	}
+
+	// No references were set, but a selector was. Select and resolve references.
+	if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels)); err != nil {
+		return MultiResolutionResponse{}, errors.Wrap(err, errListManaged)
+	}
+
+	items := req.To.List.GetItems()
+	refs := make([]v1alpha1.Reference, 0, len(items))
+	vals := make([]string, 0, len(items))
+	for _, to := range req.To.List.GetItems() {
+		if ControllersMustMatch(req.Selector) && !meta.HaveSameController(r.from, to) {
+			continue
+		}
+
+		vals = append(vals, req.Extract(to))
+		refs = append(refs, v1alpha1.Reference{Name: to.GetName()})
+	}
+
+	rsp := MultiResolutionResponse{ResolvedValues: vals, ResolvedReferences: refs}
+	return rsp, rsp.Validate()
+}
+
 // ControllersMustMatch returns true if the supplied Selector requires that a
 // reference be to a managed resource whose controller reference matches the
 // referencing resource.


Test patch:
diff --git a/pkg/reference/reference_test.go b/pkg/reference/reference_test.go
index 82f49bc..eebdedc 100644
--- a/pkg/reference/reference_test.go
+++ b/pkg/reference/reference_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -48,6 +49,25 @@ func (fml *FakeManagedList) GetItems() []resource.Managed {
 	return fml.Items
 }
 
+func TestToAndFromPtr(t *testing.T) {
+	cases := map[string]struct {
+		want string
+	}{
+		"Zero":    {want: ""},
+		"NonZero": {want: "pointy"},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := FromPtrValue(ToPtrValue(tc.want))
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("FromPtrValue(ToPtrValue(%s): -want, +got: %s", tc.want, diff)
+
+			}
+		})
+
+	}
+}
+
 func TestResolve(t *testing.T) {
 	errBoom := errors.New("boom")
 	now := metav1.Now()
@@ -123,6 +143,26 @@ func TestResolve(t *testing.T) {
 				err: errors.Wrap(errBoom, errGetManaged),
 			},
 		},
+		"ResolvedNoValue": {
+			reason: "Should return an error if the extract function returns the empty string",
+			c: &test.MockClient{
+				MockGet: test.NewMockGetFn(nil),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: ResolutionRequest{
+					Reference: ref,
+					To:        To{Managed: &fake.Managed{}},
+					Extract:   func(resource.Managed) string { return "" },
+				},
+			},
+			want: want{
+				rsp: ResolutionResponse{
+					ResolvedReference: ref,
+				},
+				err: errors.New(errNoValue),
+			},
+		},
 		"SuccessfulResolve": {
 			reason: "No error should be returned when the value is successfully extracted",
 			c: &test.MockClient{
@@ -219,6 +259,198 @@ func TestResolve(t *testing.T) {
 		})
 	}
 }
+func TestResolveMultiple(t *testing.T) {
+	errBoom := errors.New("boom")
+	now := metav1.Now()
+	value := "coolv"
+	ref := v1alpha1.Reference{Name: "cool"}
+
+	controlled := &fake.Managed{}
+	controlled.SetName(value)
+	meta.SetExternalName(controlled, value)
+	meta.AddControllerReference(controlled, meta.AsController(&corev1.ObjectReference{UID: types.UID("very-unique")}))
+
+	type args struct {
+		ctx context.Context
+		req MultiResolutionRequest
+	}
+	type want struct {
+		rsp MultiResolutionResponse
+		err error
+	}
+	cases := map[string]struct {
+		reason string
+		c      client.Reader
+		from   resource.Managed
+		args   args
+		want   want
+	}{
+		"FromDeleted": {
+			reason: "Should return early if the referencing managed resource was deleted",
+			from:   &fake.Managed{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &now}},
+			args: args{
+				req: MultiResolutionRequest{},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{},
+				err: nil,
+			},
+		},
+		"AlreadyResolved": {
+			reason: "Should return early if the current value is non-zero",
+			from:   &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{CurrentValues: []string{value}},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{ResolvedValues: []string{value}},
+				err: nil,
+			},
+		},
+		"Unresolvable": {
+			reason: "Should return early if neither a reference or selector were provided",
+			from:   &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"GetError": {
+			reason: "Should return errors encountered while getting the referenced resource",
+			c: &test.MockClient{
+				MockGet: test.NewMockGetFn(errBoom),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{
+					References: []v1alpha1.Reference{ref},
+					To:         To{Managed: &fake.Managed{}},
+					Extract:    ExternalName(),
+				},
+			},
+			want: want{
+				err: errors.Wrap(errBoom, errGetManaged),
+			},
+		},
+		"ResolvedNoValue": {
+			reason: "Should return an error if the extract function returns the empty string",
+			c: &test.MockClient{
+				MockGet: test.NewMockGetFn(nil),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{
+					References: []v1alpha1.Reference{ref},
+					To:         To{Managed: &fake.Managed{}},
+					Extract:    func(resource.Managed) string { return "" },
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{
+					ResolvedValues:     []string{""},
+					ResolvedReferences: []v1alpha1.Reference{ref},
+				},
+				err: errors.New(errNoValue),
+			},
+		},
+		"SuccessfulResolve": {
+			reason: "No error should be returned when the value is successfully extracted",
+			c: &test.MockClient{
+				MockGet: test.NewMockGetFn(nil, func(obj runtime.Object) error {
+					meta.SetExternalName(obj.(metav1.Object), value)
+					return nil
+				}),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{
+					References: []v1alpha1.Reference{ref},
+					To:         To{Managed: &fake.Managed{}},
+					Extract:    ExternalName(),
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{
+					ResolvedValues:     []string{value},
+					ResolvedReferences: []v1alpha1.Reference{ref},
+				},
+			},
+		},
+		"ListError": {
+			reason: "Should return errors encountered while listing potential referenced resources",
+			c: &test.MockClient{
+				MockList: test.NewMockListFn(errBoom),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{
+					Selector: &v1alpha1.Selector{},
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{},
+				err: errors.Wrap(errBoom, errListManaged),
+			},
+		},
+		"NoMatches": {
+			reason: "Should return an error when no managed resources match the selector",
+			c: &test.MockClient{
+				MockList: test.NewMockListFn(nil),
+			},
+			from: &fake.Managed{},
+			args: args{
+				req: MultiResolutionRequest{
+					Selector: &v1alpha1.Selector{},
+					To:       To{List: &FakeManagedList{}},
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{},
+				err: errors.New(errNoMatches),
+			},
+		},
+		"SuccessfulSelect": {
+			reason: "A managed resource with a matching controller reference should be selected and returned",
+			c: &test.MockClient{
+				MockList: test.NewMockListFn(nil),
+			},
+			from: controlled,
+			args: args{
+				req: MultiResolutionRequest{
+					Selector: &v1alpha1.Selector{
+						MatchControllerRef: func() *bool { t := true; return &t }(),
+					},
+					To: To{List: &FakeManagedList{Items: []resource.Managed{
+						&fake.Managed{}, // A resource that does not match.
+						controlled,      // A resource with a matching controller reference.
+					}}},
+					Extract: ExternalName(),
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{
+					ResolvedValues:     []string{value},
+					ResolvedReferences: []v1alpha1.Reference{{Name: value}},
+				},
+				err: nil,
+			},
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			r := NewAPIResolver(tc.c, tc.from)
+			got, err := r.ResolveMultiple(tc.args.ctx, tc.args.req)
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nControllersMustMatch(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.rsp, got, cmpopts.EquateEmpty()); diff != "" {
+				t.Errorf("\n%s\nControllersMustMatch(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}
 
 func TestControllersMustMatch(t *testing.T) {
 	cases := map[string]struct {