Skip to content

Commit 9241bce

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2120 from vincepri/rework-source-predicate-handlers
⚠️ Refactor source/handler/predicate packages to remove dep injection
2 parents 16f7965 + ea1fcf3 commit 9241bce

35 files changed

+289
-1414
lines changed

‎.golangci.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ issues:
8080
- Subprocess launch(ed with variable|ing should be audited)
8181
- (G204|G104|G307)
8282
- "ST1000: at least one file in a package should have a package comment"
83+
- "SA1019: \"sigs.k8s.io/controller-runtime/pkg/runtime/inject\""
84+
- "SA1019: inject.*"
8385
exclude-rules:
8486
- linters:
8587
- gosec

‎examples/builtins/main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,14 @@ func main() {
5959
}
6060

6161
// Watch ReplicaSets and enqueue ReplicaSet object key
62-
if err := c.Watch(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}); err != nil {
62+
if err := c.Watch(source.Kind(mgr.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}); err != nil {
6363
entryLog.Error(err, "unable to watch ReplicaSets")
6464
os.Exit(1)
6565
}
6666

6767
// Watch Pods and enqueue owning ReplicaSet key
68-
if err := c.Watch(&source.Kind{Type: &corev1.Pod{}},
69-
&handler.EnqueueRequestForOwner{OwnerType: &appsv1.ReplicaSet{}, IsController: true}); err != nil {
68+
if err := c.Watch(source.Kind(mgr.GetCache(), &corev1.Pod{}),
69+
handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &appsv1.ReplicaSet{}, handler.OnlyControllerOwner())); err != nil {
7070
entryLog.Error(err, "unable to watch Pods")
7171
os.Exit(1)
7272
}

‎hack/test-all.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ if [[ -n ${ARTIFACTS:-} ]]; then
2525
fi
2626

2727
result=0
28-
go test -race ${P_FLAG} ${MOD_OPT} ./... ${GINKGO_ARGS} || result=$?
28+
go test -v -race ${P_FLAG} ${MOD_OPT} ./... --ginkgo.fail-fast ${GINKGO_ARGS} || result=$?
2929

3030
if [[ -n ${ARTIFACTS:-} ]]; then
3131
mkdir -p ${ARTIFACTS}

‎pkg/builder/controller.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
3131
"sigs.k8s.io/controller-runtime/pkg/controller"
3232
"sigs.k8s.io/controller-runtime/pkg/handler"
33+
internalsource "sigs.k8s.io/controller-runtime/pkg/internal/source"
3334
"sigs.k8s.io/controller-runtime/pkg/manager"
3435
"sigs.k8s.io/controller-runtime/pkg/predicate"
3536
"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -217,11 +218,11 @@ func (blder *Builder) project(obj client.Object, proj objectProjection) (client.
217218
func (blder *Builder) doWatch() error {
218219
// Reconcile type
219220
if blder.forInput.object != nil {
220-
typeForSrc, err := blder.project(blder.forInput.object, blder.forInput.objectProjection)
221+
obj, err := blder.project(blder.forInput.object, blder.forInput.objectProjection)
221222
if err != nil {
222223
return err
223224
}
224-
src := &source.Kind{Type: typeForSrc}
225+
src := source.Kind(blder.mgr.GetCache(), obj)
225226
hdler := &handler.EnqueueRequestForObject{}
226227
allPredicates := append(blder.globalPredicates, blder.forInput.predicates...)
227228
if err := blder.ctrl.Watch(src, hdler, allPredicates...); err != nil {
@@ -234,15 +235,16 @@ func (blder *Builder) doWatch() error {
234235
return errors.New("Owns() can only be used together with For()")
235236
}
236237
for _, own := range blder.ownsInput {
237-
typeForSrc, err := blder.project(own.object, own.objectProjection)
238+
obj, err := blder.project(own.object, own.objectProjection)
238239
if err != nil {
239240
return err
240241
}
241-
src := &source.Kind{Type: typeForSrc}
242-
hdler := &handler.EnqueueRequestForOwner{
243-
OwnerType: blder.forInput.object,
244-
IsController: true,
245-
}
242+
src := source.Kind(blder.mgr.GetCache(), obj)
243+
hdler := handler.EnqueueRequestForOwner(
244+
blder.mgr.GetScheme(), blder.mgr.GetRESTMapper(),
245+
blder.forInput.object,
246+
handler.OnlyControllerOwner(),
247+
)
246248
allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
247249
allPredicates = append(allPredicates, own.predicates...)
248250
if err := blder.ctrl.Watch(src, hdler, allPredicates...); err != nil {
@@ -258,8 +260,8 @@ func (blder *Builder) doWatch() error {
258260
allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
259261
allPredicates = append(allPredicates, w.predicates...)
260262

261-
// If the source of this watch is of type *source.Kind, project it.
262-
if srckind, ok := w.src.(*source.Kind); ok {
263+
// If the source of this watch is of type Kind, project it.
264+
if srckind, ok := w.src.(*internalsource.Kind); ok {
263265
typeForSrc, err := blder.project(srckind.Type, w.objectProjection)
264266
if err != nil {
265267
return err

‎pkg/builder/controller_test.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ var _ = Describe("application", func() {
118118
Expect(err).NotTo(HaveOccurred())
119119

120120
instance, err := ControllerManagedBy(m).
121-
Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}).
121+
Watches(source.Kind(m.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}).
122122
Build(noop)
123123
Expect(err).To(MatchError(ContainSubstring("one of For() or Named() must be called")))
124124
Expect(instance).To(BeNil())
@@ -157,7 +157,7 @@ var _ = Describe("application", func() {
157157

158158
instance, err := ControllerManagedBy(m).
159159
Named("my_controller").
160-
Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}).
160+
Watches(source.Kind(m.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}).
161161
Build(noop)
162162
Expect(err).NotTo(HaveOccurred())
163163
Expect(instance).NotTo(BeNil())
@@ -369,8 +369,9 @@ var _ = Describe("application", func() {
369369
bldr := ControllerManagedBy(m).
370370
For(&appsv1.Deployment{}).
371371
Watches( // Equivalent of Owns
372-
&source.Kind{Type: &appsv1.ReplicaSet{}},
373-
&handler.EnqueueRequestForOwner{OwnerType: &appsv1.Deployment{}, IsController: true})
372+
source.Kind(m.GetCache(), &appsv1.ReplicaSet{}),
373+
handler.EnqueueRequestForOwner(m.GetScheme(), m.GetRESTMapper(), &appsv1.Deployment{}, handler.OnlyControllerOwner()),
374+
)
374375

375376
ctx, cancel := context.WithCancel(context.Background())
376377
defer cancel()
@@ -384,10 +385,11 @@ var _ = Describe("application", func() {
384385
bldr := ControllerManagedBy(m).
385386
Named("Deployment").
386387
Watches( // Equivalent of For
387-
&source.Kind{Type: &appsv1.Deployment{}}, &handler.EnqueueRequestForObject{}).
388+
source.Kind(m.GetCache(), &appsv1.Deployment{}), &handler.EnqueueRequestForObject{}).
388389
Watches( // Equivalent of Owns
389-
&source.Kind{Type: &appsv1.ReplicaSet{}},
390-
&handler.EnqueueRequestForOwner{OwnerType: &appsv1.Deployment{}, IsController: true})
390+
source.Kind(m.GetCache(), &appsv1.ReplicaSet{}),
391+
handler.EnqueueRequestForOwner(m.GetScheme(), m.GetRESTMapper(), &appsv1.Deployment{}, handler.OnlyControllerOwner()),
392+
)
391393

392394
ctx, cancel := context.WithCancel(context.Background())
393395
defer cancel()
@@ -481,7 +483,7 @@ var _ = Describe("application", func() {
481483
bldr := ControllerManagedBy(mgr).
482484
For(&appsv1.Deployment{}, OnlyMetadata).
483485
Owns(&appsv1.ReplicaSet{}, OnlyMetadata).
484-
Watches(&source.Kind{Type: &appsv1.StatefulSet{}},
486+
Watches(source.Kind(mgr.GetCache(), &appsv1.StatefulSet{}),
485487
handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request {
486488
defer GinkgoRecover()
487489

‎pkg/cluster/cluster.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,12 @@ import (
3737

3838
// Cluster provides various methods to interact with a cluster.
3939
type Cluster interface {
40-
// SetFields will set any dependencies on an object for which the object has implemented the inject
41-
// interface - e.g. inject.Client.
42-
// Deprecated: use the equivalent Options field to set a field. This method will be removed in v0.10.
43-
SetFields(interface{}) error
44-
4540
// GetConfig returns an initialized Config
4641
GetConfig() *rest.Config
4742

43+
// GetCache returns a cache.Cache
44+
GetCache() cache.Cache
45+
4846
// GetScheme returns an initialized Scheme
4947
GetScheme() *runtime.Scheme
5048

@@ -57,9 +55,6 @@ type Cluster interface {
5755
// GetFieldIndexer returns a client.FieldIndexer configured with the client
5856
GetFieldIndexer() client.FieldIndexer
5957

60-
// GetCache returns a cache.Cache
61-
GetCache() cache.Cache
62-
6358
// GetEventRecorderFor returns a new EventRecorder for the provided name
6459
GetEventRecorderFor(name string) record.EventRecorder
6560

‎pkg/cluster/cluster_test.go

Lines changed: 0 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,8 @@ import (
2929
"k8s.io/apimachinery/pkg/runtime"
3030
"k8s.io/client-go/rest"
3131
"sigs.k8s.io/controller-runtime/pkg/cache"
32-
"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
3332
"sigs.k8s.io/controller-runtime/pkg/client"
34-
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
3533
intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
36-
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
3734
)
3835

3936
var _ = Describe("cluster.Cluster", func() {
@@ -111,78 +108,6 @@ var _ = Describe("cluster.Cluster", func() {
111108
})
112109
})
113110

114-
Describe("SetFields", func() {
115-
It("should inject field values", func() {
116-
c, err := New(cfg, func(o *Options) {
117-
o.NewCache = func(_ *rest.Config, _ cache.Options) (cache.Cache, error) {
118-
return &informertest.FakeInformers{}, nil
119-
}
120-
})
121-
Expect(err).NotTo(HaveOccurred())
122-
123-
By("Injecting the dependencies")
124-
err = c.SetFields(&injectable{
125-
scheme: func(scheme *runtime.Scheme) error {
126-
defer GinkgoRecover()
127-
Expect(scheme).To(Equal(c.GetScheme()))
128-
return nil
129-
},
130-
config: func(config *rest.Config) error {
131-
defer GinkgoRecover()
132-
Expect(config).To(Equal(c.GetConfig()))
133-
return nil
134-
},
135-
client: func(client client.Client) error {
136-
defer GinkgoRecover()
137-
Expect(client).To(Equal(c.GetClient()))
138-
return nil
139-
},
140-
cache: func(cache cache.Cache) error {
141-
defer GinkgoRecover()
142-
Expect(cache).To(Equal(c.GetCache()))
143-
return nil
144-
},
145-
log: func(logger logr.Logger) error {
146-
defer GinkgoRecover()
147-
Expect(logger).To(Equal(logf.RuntimeLog.WithName("cluster")))
148-
return nil
149-
},
150-
})
151-
Expect(err).NotTo(HaveOccurred())
152-
153-
By("Returning an error if dependency injection fails")
154-
155-
expected := fmt.Errorf("expected error")
156-
err = c.SetFields(&injectable{
157-
client: func(client client.Client) error {
158-
return expected
159-
},
160-
})
161-
Expect(err).To(Equal(expected))
162-
163-
err = c.SetFields(&injectable{
164-
scheme: func(scheme *runtime.Scheme) error {
165-
return expected
166-
},
167-
})
168-
Expect(err).To(Equal(expected))
169-
170-
err = c.SetFields(&injectable{
171-
config: func(config *rest.Config) error {
172-
return expected
173-
},
174-
})
175-
Expect(err).To(Equal(expected))
176-
177-
err = c.SetFields(&injectable{
178-
cache: func(c cache.Cache) error {
179-
return expected
180-
},
181-
})
182-
Expect(err).To(Equal(expected))
183-
})
184-
})
185-
186111
It("should not leak goroutines when stopped", func() {
187112
currentGRs := goleak.IgnoreCurrent()
188113

@@ -242,56 +167,3 @@ var _ = Describe("cluster.Cluster", func() {
242167
Expect(c.GetAPIReader()).NotTo(BeNil())
243168
})
244169
})
245-
246-
var _ inject.Cache = &injectable{}
247-
var _ inject.Client = &injectable{}
248-
var _ inject.Scheme = &injectable{}
249-
var _ inject.Config = &injectable{}
250-
var _ inject.Logger = &injectable{}
251-
252-
type injectable struct {
253-
scheme func(scheme *runtime.Scheme) error
254-
client func(client.Client) error
255-
config func(config *rest.Config) error
256-
cache func(cache.Cache) error
257-
log func(logger logr.Logger) error
258-
}
259-
260-
func (i *injectable) InjectCache(c cache.Cache) error {
261-
if i.cache == nil {
262-
return nil
263-
}
264-
return i.cache(c)
265-
}
266-
267-
func (i *injectable) InjectConfig(config *rest.Config) error {
268-
if i.config == nil {
269-
return nil
270-
}
271-
return i.config(config)
272-
}
273-
274-
func (i *injectable) InjectClient(c client.Client) error {
275-
if i.client == nil {
276-
return nil
277-
}
278-
return i.client(c)
279-
}
280-
281-
func (i *injectable) InjectScheme(scheme *runtime.Scheme) error {
282-
if i.scheme == nil {
283-
return nil
284-
}
285-
return i.scheme(scheme)
286-
}
287-
288-
func (i *injectable) InjectLogger(log logr.Logger) error {
289-
if i.log == nil {
290-
return nil
291-
}
292-
return i.log(log)
293-
}
294-
295-
func (i *injectable) Start(<-chan struct{}) error {
296-
return nil
297-
}

‎pkg/cluster/internal.go

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828
"sigs.k8s.io/controller-runtime/pkg/cache"
2929
"sigs.k8s.io/controller-runtime/pkg/client"
3030
intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
31-
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
3231
)
3332

3433
type cluster struct {
@@ -64,28 +63,6 @@ type cluster struct {
6463
logger logr.Logger
6564
}
6665

67-
func (c *cluster) SetFields(i interface{}) error {
68-
if _, err := inject.ConfigInto(c.config, i); err != nil {
69-
return err
70-
}
71-
if _, err := inject.ClientInto(c.client, i); err != nil {
72-
return err
73-
}
74-
if _, err := inject.APIReaderInto(c.apiReader, i); err != nil {
75-
return err
76-
}
77-
if _, err := inject.SchemeInto(c.scheme, i); err != nil {
78-
return err
79-
}
80-
if _, err := inject.CacheInto(c.cache, i); err != nil {
81-
return err
82-
}
83-
if _, err := inject.MapperInto(c.mapper, i); err != nil {
84-
return err
85-
}
86-
return nil
87-
}
88-
8966
func (c *cluster) GetConfig() *rest.Config {
9067
return c.config
9168
}

‎pkg/controller/controller.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,6 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
139139
options.RateLimiter = workqueue.DefaultControllerRateLimiter()
140140
}
141141

142-
// Inject dependencies into Reconciler
143-
if err := mgr.SetFields(options.Reconciler); err != nil {
144-
return nil, err
145-
}
146-
147142
if options.RecoverPanic == nil {
148143
options.RecoverPanic = mgr.GetControllerOptions().RecoverPanic
149144
}
@@ -156,7 +151,6 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
156151
},
157152
MaxConcurrentReconciles: options.MaxConcurrentReconciles,
158153
CacheSyncTimeout: options.CacheSyncTimeout,
159-
SetFields: mgr.SetFields,
160154
Name: name,
161155
LogConstructor: options.LogConstructor,
162156
RecoverPanic: options.RecoverPanic,

‎pkg/controller/controller_integration_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,13 @@ var _ = Describe("controller", func() {
6464
Expect(err).NotTo(HaveOccurred())
6565

6666
By("Watching Resources")
67-
err = instance.Watch(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForOwner{
68-
OwnerType: &appsv1.Deployment{},
69-
})
67+
err = instance.Watch(
68+
source.Kind(cm.GetCache(), &appsv1.ReplicaSet{}),
69+
handler.EnqueueRequestForOwner(cm.GetScheme(), cm.GetRESTMapper(), &appsv1.Deployment{}),
70+
)
7071
Expect(err).NotTo(HaveOccurred())
7172

72-
err = instance.Watch(&source.Kind{Type: &appsv1.Deployment{}}, &handler.EnqueueRequestForObject{})
73+
err = instance.Watch(source.Kind(cm.GetCache(), &appsv1.Deployment{}), &handler.EnqueueRequestForObject{})
7374
Expect(err).NotTo(HaveOccurred())
7475

7576
err = cm.GetClient().Get(ctx, types.NamespacedName{Name: "foo"}, &corev1.Namespace{})

0 commit comments

Comments
 (0)