Expand the tracker interface to include a variant with its own type. (#860)

`TrackReference` is the same as `Track`, but takes a `tracker.Reference` instead.  This type has been seeded with the subset of `corev1.ObjectReference` that the tracker currently consumes / supports, but the intention is to expand this type to allow inexact references that (optionally) use label selectors in place of name to reference objects.

See also: https://github.com/knative/pkg/issues/859

reconciler/testing/tracker.go

@@ -20,7 +20,6 @@ import (
 	"sync"
 
 	corev1 "k8s.io/api/core/v1"
-
 	"knative.dev/pkg/tracker"
 )
 
@@ -32,7 +31,7 @@ type NullTracker = FakeTracker
 // FakeTracker implements Tracker.
 type FakeTracker struct {
 	sync.Mutex
-	references []corev1.ObjectReference
+	references []tracker.Reference
 }
 
 var _ tracker.Interface = (*FakeTracker)(nil)
@@ -40,8 +39,18 @@ var _ tracker.Interface = (*FakeTracker)(nil)
 // OnChanged implements OnChanged.
 func (*FakeTracker) OnChanged(interface{}) {}
 
-// Track implements Track.
+// Track implements tracker.Interface.
 func (n *FakeTracker) Track(ref corev1.ObjectReference, obj interface{}) error {
+	return n.TrackReference(tracker.Reference{
+		APIVersion: ref.APIVersion,
+		Kind:       ref.Kind,
+		Namespace:  ref.Namespace,
+		Name:       ref.Name,
+	}, obj)
+}
+
+// TrackReference implements tracker.Interface.
+func (n *FakeTracker) TrackReference(ref tracker.Reference, obj interface{}) error {
 	n.Lock()
 	defer n.Unlock()
 
@@ -50,7 +59,7 @@ func (n *FakeTracker) Track(ref corev1.ObjectReference, obj interface{}) error {
 }
 
 // References returns the list of objects being tracked
-func (n *FakeTracker) References() []corev1.ObjectReference {
+func (n *FakeTracker) References() []tracker.Reference {
 	n.Lock()
 	defer n.Unlock()
 

tracker/enqueue.go

@@ -49,7 +49,7 @@ type impl struct {
 	m sync.Mutex
 	// mapping maps from an object reference to the set of
 	// keys for objects watching it.
-	mapping map[corev1.ObjectReference]set
+	mapping map[Reference]set
 
 	// The amount of time that an object may watch another
 	// before having to renew the lease.
@@ -66,6 +66,15 @@ type set map[types.NamespacedName]time.Time
 
 // Track implements Interface.
 func (i *impl) Track(ref corev1.ObjectReference, obj interface{}) error {
+	return i.TrackReference(Reference{
+		APIVersion: ref.APIVersion,
+		Kind:       ref.Kind,
+		Namespace:  ref.Namespace,
+		Name:       ref.Name,
+	}, obj)
+}
+
+func (i *impl) TrackReference(ref Reference, obj interface{}) error {
 	invalidFields := map[string][]string{
 		"APIVersion": validation.IsQualifiedName(ref.APIVersion),
 		"Kind":       validation.IsCIdentifier(ref.Kind),
@@ -93,7 +102,7 @@ func (i *impl) Track(ref corev1.ObjectReference, obj interface{}) error {
 	i.m.Lock()
 	defer i.m.Unlock()
 	if i.mapping == nil {
-		i.mapping = make(map[corev1.ObjectReference]set)
+		i.mapping = make(map[Reference]set)
 	}
 
 	l, ok := i.mapping[ref]
@@ -134,12 +143,18 @@ func (i *impl) OnChanged(obj interface{}) {
 	}
 
 	or := kmeta.ObjectReference(item)
+	ref := Reference{
+		APIVersion: or.APIVersion,
+		Kind:       or.Kind,
+		Namespace:  or.Namespace,
+		Name:       or.Name,
+	}
 
 	// TODO(mattmoor): Consider locking the mapping (global) for a
 	// smaller scope and leveraging a per-set lock to guard its access.
 	i.m.Lock()
 	defer i.m.Unlock()
-	s, ok := i.mapping[or]
+	s, ok := i.mapping[ref]
 	if !ok {
 		// TODO(mattmoor): We should consider logging here.
 		return
@@ -155,6 +170,6 @@ func (i *impl) OnChanged(obj interface{}) {
 	}
 
 	if len(s) == 0 {
-		delete(i.mapping, or)
+		delete(i.mapping, ref)
 	}
 }

tracker/enqueue_test.go

@@ -48,7 +48,13 @@ func TestHappyPaths(t *testing.T) {
 			Name:      "foo",
 		},
 	}
-	objRef := kmeta.ObjectReference(thing1)
+	or := kmeta.ObjectReference(thing1)
+	ref := Reference{
+		APIVersion: or.APIVersion,
+		Kind:       or.Kind,
+		Namespace:  or.Namespace,
+		Name:       or.Name,
+	}
 
 	thing2 := &Resource{
 		TypeMeta: metav1.TypeMeta{
@@ -71,7 +77,7 @@ func TestHappyPaths(t *testing.T) {
 
 	// Tracked gets called
 	{
-		if err := trk.Track(objRef, thing2); err != nil {
+		if err := trk.Track(ref.ObjectReference(), thing2); err != nil {
 			t.Errorf("Track() = %v", err)
 		}
 		// New registrations should result in an immediate callback.
@@ -100,14 +106,14 @@ func TestHappyPaths(t *testing.T) {
 		if got, want := calls, 3; got != want {
 			t.Errorf("OnChanged() = %v, wanted %v", got, want)
 		}
-		if _, stillThere := trk.(*impl).mapping[objRef]; stillThere {
+		if _, stillThere := trk.(*impl).mapping[ref]; stillThere {
 			t.Error("Timeout passed, but mapping for objectReference is still there")
 		}
 	}
 
 	// Starts getting called again
 	{
-		if err := trk.Track(objRef, thing2); err != nil {
+		if err := trk.Track(ref.ObjectReference(), thing2); err != nil {
 			t.Errorf("Track() = %v", err)
 		}
 		// New registrations should result in an immediate callback.
@@ -160,7 +166,7 @@ func TestHappyPaths(t *testing.T) {
 
 	// Track bad object
 	{
-		if err := trk.Track(objRef, struct{}{}); err == nil {
+		if err := trk.Track(ref.ObjectReference(), struct{}{}); err == nil {
 			t.Error("Track() = nil, wanted error")
 		}
 	}

tracker/interface.go

@@ -17,17 +17,103 @@ limitations under the License.
 package tracker
 
 import (
+	"context"
+	"strings"
+
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/validation"
+
+	"knative.dev/pkg/apis"
 )
 
+// Reference is modeled after corev1.ObjectReference, but omits fields
+// unsupported by the tracker, and permits us to extend things in
+// divergent ways.
+type Reference struct {
+	// API version of the referent.
+	// +optional
+	APIVersion string `json:"apiVersion,omitempty"`
+
+	// Kind of the referent.
+	// +optional
+	Kind string `json:"kind,omitempty"`
+
+	// Namespace of the referent.
+	// +optional
+	Namespace string `json:"namespace,omitempty"`
+
+	// Name of the referent.
+	// +optional
+	Name string `json:"name,omitempty"`
+}
+
 // Interface defines the interface through which an object can register
 // that it is tracking another object by reference.
 type Interface interface {
 	// Track tells us that "obj" is tracking changes to the
 	// referenced object.
+	// DEPRECATED: use TrackReference
 	Track(ref corev1.ObjectReference, obj interface{}) error
 
+	// Track tells us that "obj" is tracking changes to the
+	// referenced object.
+	TrackReference(ref Reference, obj interface{}) error
+
 	// OnChanged is a callback to register with the InformerFactory
 	// so that we are notified for appropriate object changes.
 	OnChanged(obj interface{})
 }
+
+// GroupVersionKind returns the GroupVersion of the object referenced.
+func (ref *Reference) GroupVersionKind() schema.GroupVersionKind {
+	gv, _ := schema.ParseGroupVersion(ref.APIVersion)
+	return schema.GroupVersionKind{
+		Group:   gv.Group,
+		Version: gv.Version,
+		Kind:    ref.Kind,
+	}
+}
+
+// ObjectReference returns the tracker Reference as an ObjectReference.
+func (ref *Reference) ObjectReference() corev1.ObjectReference {
+	return corev1.ObjectReference{
+		APIVersion: ref.APIVersion,
+		Kind:       ref.Kind,
+		Namespace:  ref.Namespace,
+		Name:       ref.Name,
+	}
+}
+
+// ValidateObjectReference validates that the Reference uses a subset suitable for
+// translation to a corev1.ObjectReference.  This helper is intended to simplify
+// validating a particular (narrow) use of tracker.Reference.
+func (ref *Reference) ValidateObjectReference(ctx context.Context) *apis.FieldError {
+	var errs *apis.FieldError
+
+	// Required fields
+	if ref.APIVersion == "" {
+		errs = errs.Also(apis.ErrMissingField("apiVersion"))
+	} else if verrs := validation.IsQualifiedName(ref.APIVersion); len(verrs) != 0 {
+		errs = errs.Also(apis.ErrInvalidValue(strings.Join(verrs, ", "), "apiVersion"))
+	}
+	if ref.Kind == "" {
+		errs = errs.Also(apis.ErrMissingField("kind"))
+	} else if verrs := validation.IsCIdentifier(ref.Kind); len(verrs) != 0 {
+		errs = errs.Also(apis.ErrInvalidValue(strings.Join(verrs, ", "), "kind"))
+	}
+	if ref.Name == "" {
+		errs = errs.Also(apis.ErrMissingField("name"))
+	} else if verrs := validation.IsDNS1123Label(ref.Name); len(verrs) != 0 {
+		errs = errs.Also(apis.ErrInvalidValue(strings.Join(verrs, ", "), "name"))
+	}
+	if ref.Namespace == "" {
+		errs = errs.Also(apis.ErrMissingField("namespace"))
+	} else if verrs := validation.IsDNS1123Label(ref.Namespace); len(verrs) != 0 {
+		errs = errs.Also(apis.ErrInvalidValue(strings.Join(verrs, ", "), "namespace"))
+	}
+
+	// TODO(mattmoor): Add disallowed fields here
+
+	return errs
+}

tracker/interface_test.go

@@ -0,0 +1,115 @@
+/*
+Copyright 2019 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tracker
+
+import (
+	"context"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/apis"
+)
+
+func TestGroupVersionKind(t *testing.T) {
+	ref := Reference{
+		APIVersion: "apps/v1",
+		Kind:       "Deployment",
+		Namespace:  "default",
+		Name:       "nginx",
+	}
+
+	want := schema.GroupVersionKind{
+		Group:   "apps",
+		Version: "v1",
+		Kind:    "Deployment",
+	}
+	got := ref.GroupVersionKind()
+	if want != got {
+		t.Errorf("GroupVersionKind() = %v, wanted = %v", got, want)
+	}
+}
+
+func TestValidateObjectReference(t *testing.T) {
+	tests := []struct {
+		name string
+		ref  Reference
+		want *apis.FieldError
+	}{{
+		name: "empty reference",
+		want: apis.ErrMissingField("apiVersion", "kind", "name", "namespace"),
+	}, {
+		name: "good reference",
+		ref: Reference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Namespace:  "default",
+			Name:       "nginx",
+		},
+	}, {
+		name: "another good reference",
+		ref: Reference{
+			APIVersion: "v1",
+			Kind:       "Service",
+			Namespace:  "default",
+			Name:       "nginx",
+		},
+	}, {
+		name: "bad apiVersion",
+		ref: Reference{
+			APIVersion: "a b c d", // Bad!
+			Kind:       "Service",
+			Namespace:  "default",
+			Name:       "nginx",
+		},
+		want: apis.ErrInvalidValue("name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')", "apiVersion"),
+	}, {
+		name: "bad kind",
+		ref: Reference{
+			APIVersion: "apps/v1",
+			Kind:       "a b c d", // Bad!
+			Namespace:  "default",
+			Name:       "nginx",
+		},
+		want: apis.ErrInvalidValue("a valid C identifier must start with alphabetic character or '_', followed by a string of alphanumeric characters or '_' (e.g. 'my_name',  or 'MY_NAME',  or 'MyName', regex used for validation is '[A-Za-z_][A-Za-z0-9_]*')", "kind"),
+	}, {
+		name: "bad namespace and name",
+		ref: Reference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Namespace:  "a.b",
+			Name:       "c.d",
+		},
+		want: &apis.FieldError{
+			Message: "invalid value: a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name',  or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')",
+			Paths:   []string{"namespace", "name"},
+		},
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := test.ref.ValidateObjectReference(context.Background())
+			if (test.want != nil) != (got != nil) {
+				t.Errorf("ValidateObjectReference() = %v, wanted %v", got, test.want)
+			} else if test.want != nil {
+				want, got := test.want.Error(), got.Error()
+				if got != want {
+					t.Errorf("ValidateObjectReference() = %s, wanted %s", got, want)
+				}
+			}
+		})
+	}
+}