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.

Sign up for GitHub

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

Jump to bottom

🐛 EnqueueRequestForOwner correctly enqueue cluster-scoped owner #274

Merged
merged 1 commit into from
Jan 16, 2019
Merged

🐛 EnqueueRequestForOwner correctly enqueue cluster-scoped owner #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 27 additions & 4 deletions pkg/handler/enqueue_owner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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
}
13 changes: 13 additions & 0 deletions pkg/handler/eventhandler_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just extract this out onto a helper on testenv at some point? I want to be able to call BeforeSuite(testenv.Environment{}.BeforeSuite) or something similar.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create an issue to track this or is it already tracked some other place?

})

var _ = AfterSuite(func() {
testenv.Stop()
})
57 changes: 54 additions & 3 deletions pkg/handler/eventhandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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() {
Expand Down Expand Up @@ -347,6 +356,7 @@ var _ = Describe("Eventhandler", func() {
OwnerType: &appsv1.ReplicaSet{},
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)

pod.OwnerReferences = []metav1.OwnerReference{
{
Expand All @@ -372,6 +382,7 @@ var _ = Describe("Eventhandler", func() {
OwnerType: &appsv1.ReplicaSet{},
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)

pod.OwnerReferences = []metav1.OwnerReference{
{
Expand Down Expand Up @@ -401,6 +412,7 @@ var _ = Describe("Eventhandler", func() {
OwnerType: &appsv1.ReplicaSet{},
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)

pod.OwnerReferences = []metav1.OwnerReference{
{
Expand Down Expand Up @@ -439,6 +451,7 @@ var _ = Describe("Eventhandler", func() {
OwnerType: &appsv1.ReplicaSet{},
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)

pod.OwnerReferences = []metav1.OwnerReference{
{
Expand All @@ -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",
Expand All @@ -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{
Expand All @@ -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(),
Expand All @@ -531,6 +573,7 @@ var _ = Describe("Eventhandler", func() {
IsController: t,
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)
pod.OwnerReferences = []metav1.OwnerReference{
{
Name: "foo1-parent",
Expand Down Expand Up @@ -577,6 +620,7 @@ var _ = Describe("Eventhandler", func() {
IsController: t,
}
instance.InjectScheme(scheme.Scheme)
instance.InjectMapper(mapper)
pod.OwnerReferences = []metav1.OwnerReference{
{
Name: "foo1-parent",
Expand Down Expand Up @@ -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(),
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions pkg/manager/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
15 changes: 15 additions & 0 deletions pkg/runtime/inject/inject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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

Expand Down