Merge pull request #274 from shawn-hurley/feature/issue-228

🐛 EnqueueRequestForOwner correctly enqueue cluster-scoped owner

Author: k8s-ci-robot

Gopkg.lock

@@ -19,6 +19,7 @@ package handler
 import (
 "fmt"
 
+"k8s.io/apimachinery/pkg/api/meta"
 metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 "k8s.io/apimachinery/pkg/runtime"
 "k8s.io/apimachinery/pkg/runtime/schema"
@@ -51,6 +52,9 @@ type EnqueueRequestForOwner struct {
 
 // groupKind is the cached Group and Kind from OwnerType
 groupKind schema.GroupKind
+
+// mapper maps GroupVersionKinds to Resources
+mapper meta.RESTMapper
 }
 
 // Create implements EventHandler
@@ -126,10 +130,21 @@ func (e *EnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object)
 // object in the event.
 if ref.Kind == e.groupKind.Kind && refGV.Group == e.groupKind.Group {
 // Match found - add a Request for the object referred to in the OwnerReference
-result = append(result, reconcile.Request{NamespacedName: types.NamespacedName{
-Namespace: object.GetNamespace(),
-Name: ref.Name,
-}})
+request := reconcile.Request{NamespacedName: types.NamespacedName{
+Name: ref.Name,
+}}
+
+// if owner is not namespaced then we should set the namespace to the empty
+mapping, err := e.mapper.RESTMapping(e.groupKind, refGV.Version)
+if err != nil {
+log.Error(err, "Could not retrieve rest mapping", "kind", e.groupKind)
+return nil
+}
+if mapping.Scope.Name() != meta.RESTScopeNameRoot {
+request.Namespace = object.GetNamespace()
+}
+
+result = append(result, request)
 }
 }
 
@@ -163,3 +178,11 @@ var _ inject.Scheme = &EnqueueRequestForOwner{}
 func (e *EnqueueRequestForOwner) InjectScheme(s *runtime.Scheme) error {
 return e.parseOwnerTypeGroupKind(s)
 }
+
+var _ inject.Mapper = &EnqueueRequestForOwner{}
+
+// InjectMapper is called by the Controller to provide the rest mapper used by the manager.
+func (e *EnqueueRequestForOwner) InjectMapper(m meta.RESTMapper) error {
+e.mapper = m
+return nil
+}

pkg/handler/enqueue_owner.go

@@ -21,6 +21,7 @@ import (
 
 . "github.com/onsi/ginkgo"
 . "github.com/onsi/gomega"
+"k8s.io/client-go/rest"
 "sigs.k8s.io/controller-runtime/pkg/envtest"
 logf "sigs.k8s.io/controller-runtime/pkg/log"
 "sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -31,6 +32,18 @@ func TestEventhandler(t *testing.T) {
 RunSpecsWithDefaultAndCustomReporters(t, "Eventhandler Suite", []Reporter{envtest.NewlineReporter{}})
 }
 
+var testenv *envtest.Environment
+var cfg *rest.Config
+
 var _ = BeforeSuite(func() {
 logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))
+
+testenv = &envtest.Environment{}
+var err error
+cfg, err = testenv.Start()
+Expect(err).NotTo(HaveOccurred())
+})
+
+var _ = AfterSuite(func() {
+testenv.Stop()
 })

pkg/handler/eventhandler_suite_test.go

@@ -20,11 +20,14 @@ import (
 . "github.com/onsi/ginkgo"
 . "github.com/onsi/gomega"
 appsv1 "k8s.io/api/apps/v1"
+autoscalingv1 "k8s.io/api/autoscaling/v1"
 corev1 "k8s.io/api/core/v1"
+"k8s.io/apimachinery/pkg/api/meta"
 metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 "k8s.io/apimachinery/pkg/types"
 "k8s.io/client-go/kubernetes/scheme"
 "k8s.io/client-go/util/workqueue"
+"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 "sigs.k8s.io/controller-runtime/pkg/controller/controllertest"
 "sigs.k8s.io/controller-runtime/pkg/event"
 "sigs.k8s.io/controller-runtime/pkg/handler"
@@ -35,13 +38,19 @@ var _ = Describe("Eventhandler", func() {
 var q workqueue.RateLimitingInterface
 var instance handler.EnqueueRequestForObject
 var pod *corev1.Pod
+var mapper meta.RESTMapper
 t := true
 BeforeEach(func() {
 q = controllertest.Queue{Interface: workqueue.New()}
 instance = handler.EnqueueRequestForObject{}
 pod = &corev1.Pod{
 ObjectMeta: metav1.ObjectMeta{Namespace: "biz", Name: "baz"},
 }
+Expect(cfg).NotTo(BeNil())
+
+var err error
+mapper, err = apiutil.NewDiscoveryRESTMapper(cfg)
+Expect(err).ShouldNot(HaveOccurred())
 })
 
 Describe("EnqueueRequestForObject", func() {
@@ -347,6 +356,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 
 pod.OwnerReferences = []metav1.OwnerReference{
 {
@@ -372,6 +382,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 
 pod.OwnerReferences = []metav1.OwnerReference{
 {
@@ -401,6 +412,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 
 pod.OwnerReferences = []metav1.OwnerReference{
 {
@@ -439,6 +451,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 
 pod.OwnerReferences = []metav1.OwnerReference{
 {
@@ -465,6 +478,7 @@ var _ = Describe("Eventhandler", func() {
 IsController: t,
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 { // Wrong group
 Name: "foo1-parent",
@@ -488,14 +502,15 @@ var _ = Describe("Eventhandler", func() {
 It("should enqueue a Request if there are owners matching Group "+
 "and Kind with a different version.", func() {
 instance := handler.EnqueueRequestForOwner{
-OwnerType: &appsv1.ReplicaSet{},
+OwnerType: &autoscalingv1.HorizontalPodAutoscaler{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo-parent",
-Kind: "ReplicaSet",
-APIVersion: "apps/v2",
+Kind: "HorizontalPodAutoscaler",
+APIVersion: "autoscaling/v2beta1",
 },
 }
 evt := event.CreateEvent{
@@ -510,11 +525,38 @@ var _ = Describe("Eventhandler", func() {
 NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo-parent"}}))
 })
 
+It("should enqueue a Request for a owner that is cluster scoped", func() {
+instance := handler.EnqueueRequestForOwner{
+OwnerType: &corev1.Node{},
+}
+instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
+pod.OwnerReferences = []metav1.OwnerReference{
+{
+Name: "node-1",
+Kind: "Node",
+APIVersion: "v1",
+},
+}
+evt := event.CreateEvent{
+Object: pod,
+Meta: pod.GetObjectMeta(),
+}
+instance.Create(evt, q)
+Expect(q.Len()).To(Equal(1))
+
+i, _ := q.Get()
+Expect(i).To(Equal(reconcile.Request{
+NamespacedName: types.NamespacedName{Namespace: "", Name: "node-1"}}))
+
+})
+
 It("should not enqueue a Request if there are no owners.", func() {
 instance := handler.EnqueueRequestForOwner{
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 evt := event.CreateEvent{
 Object: pod,
 Meta: pod.GetObjectMeta(),
@@ -531,6 +573,7 @@ var _ = Describe("Eventhandler", func() {
 IsController: t,
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -577,6 +620,7 @@ var _ = Describe("Eventhandler", func() {
 IsController: t,
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -608,6 +652,7 @@ var _ = Describe("Eventhandler", func() {
 IsController: t,
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 evt := event.CreateEvent{
 Object: pod,
 Meta: pod.GetObjectMeta(),
@@ -623,6 +668,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -665,6 +711,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -686,6 +733,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &metav1.ListOptions{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -707,6 +755,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &controllertest.ErrorType{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -727,6 +776,7 @@ var _ = Describe("Eventhandler", func() {
 It("should do nothing.", func() {
 instance := handler.EnqueueRequestForOwner{}
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",
@@ -749,6 +799,7 @@ var _ = Describe("Eventhandler", func() {
 OwnerType: &appsv1.ReplicaSet{},
 }
 instance.InjectScheme(scheme.Scheme)
+instance.InjectMapper(mapper)
 pod.OwnerReferences = []metav1.OwnerReference{
 {
 Name: "foo1-parent",

pkg/handler/eventhandler_test.go

@@ -139,6 +139,9 @@ func (cm *controllerManager) SetFields(i interface{}) error {
 if _, err := inject.DecoderInto(cm.admissionDecoder, i); err != nil {
 return err
 }
+if _, err := inject.MapperInto(cm.mapper, i); err != nil {
+return err
+}
 return nil
 }
 

pkg/manager/internal.go

@@ -17,6 +17,7 @@ limitations under the License.
 package inject
 
 import (
+"k8s.io/apimachinery/pkg/api/meta"
 "k8s.io/apimachinery/pkg/runtime"
 "k8s.io/client-go/rest"
 "sigs.k8s.io/controller-runtime/pkg/cache"
@@ -113,6 +114,20 @@ func StopChannelInto(stop <-chan struct{}, i interface{}) (bool, error) {
 return false, nil
 }
 
+// Mapper is used to inject the rest mapper to components that may need it
+type Mapper interface {
+InjectMapper(meta.RESTMapper) error
+}
+
+// MapperInto will set the rest mapper on i and return the result if it implements Mapper.
+// Returns false if i does not implement Mapper.
+func MapperInto(mapper meta.RESTMapper, i interface{}) (bool, error) {
+if m, ok := i.(Mapper); ok {
+return true, m.InjectMapper(mapper)
+}
+return false, nil
+}
+
 // Func injects dependencies into i.
 type Func func(i interface{}) error