From 7a5987b667ce3076f86443770cf1e7efd6ced69a Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Thu, 17 Jul 2025 13:00:39 -0700 Subject: [PATCH 1/4] chore: Get resource from cache instead of APIServer Currently in ACK, at the beginning of the Reconciler for each event, we get the resource from the k8s APIServer. This is an unecessary call, as the latest object that triggered the reconciler is already stored in the informers cache. With this change, we will be retrieving the resource from the cache instead of the APIServer. --- Makefile | 3 + mocks/controller-runtime/pkg/cache/cache.go | 231 ++++++++++++++++++ .../pkg/cache/new_cache_func.go | 47 ++++ pkg/runtime/adoption_reconciler.go | 13 +- pkg/runtime/adoption_reconciler_test.go | 59 ++--- pkg/runtime/field_export_reconciler.go | 49 ++-- pkg/runtime/field_export_reconciler_test.go | 53 ++-- pkg/runtime/reconciler.go | 29 +-- pkg/runtime/reconciler_test.go | 2 +- 9 files changed, 386 insertions(+), 100 deletions(-) create mode 100644 mocks/controller-runtime/pkg/cache/cache.go create mode 100644 mocks/controller-runtime/pkg/cache/new_cache_func.go diff --git a/Makefile b/Makefile index 88f2fbbf..b354d4ce 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,9 @@ mocks: install-mockery ## Build mocks @echo -n "building mocks for sigs.k8s.io/controller-runtime/pkg/client ... " @bin/mockery --quiet --name="(Object|Client|Status|Reader|SubResourceWriter)" --case=underscore --output=mocks/controller-runtime/pkg/client --dir="$(CONTROLLER_RUNTIME_DIR)/pkg/client" @echo "ok." + @echo -n "building mocks for sigs.k8s.io/controller-runtime/pkg/cache ... " + @bin/mockery --quiet --name="(Cache)" --case=underscore --output=mocks/controller-runtime/pkg/cache --dir="$(CONTROLLER_RUNTIME_DIR)/pkg/cache" + @echo "ok." help: ## Show this help. @grep -F -h "##" $(MAKEFILE_LIST) | grep -F -v grep | sed -e 's/\\$$//' \ diff --git a/mocks/controller-runtime/pkg/cache/cache.go b/mocks/controller-runtime/pkg/cache/cache.go new file mode 100644 index 00000000..3681484e --- /dev/null +++ b/mocks/controller-runtime/pkg/cache/cache.go @@ -0,0 +1,231 @@ +// Code generated by mockery v2.53.3. DO NOT EDIT. + +package mocks + +import ( + cache "sigs.k8s.io/controller-runtime/pkg/cache" + client "sigs.k8s.io/controller-runtime/pkg/client" + + context "context" + + mock "github.com/stretchr/testify/mock" + + schema "k8s.io/apimachinery/pkg/runtime/schema" + + types "k8s.io/apimachinery/pkg/types" +) + +// Cache is an autogenerated mock type for the Cache type +type Cache struct { + mock.Mock +} + +// Get provides a mock function with given fields: ctx, key, obj, opts +func (_m *Cache) Get(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, key, obj) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for Get") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, types.NamespacedName, client.Object, ...client.GetOption) error); ok { + r0 = rf(ctx, key, obj, opts...) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// GetInformer provides a mock function with given fields: ctx, obj, opts +func (_m *Cache) GetInformer(ctx context.Context, obj client.Object, opts ...cache.InformerGetOption) (cache.Informer, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, obj) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for GetInformer") + } + + var r0 cache.Informer + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, client.Object, ...cache.InformerGetOption) (cache.Informer, error)); ok { + return rf(ctx, obj, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, client.Object, ...cache.InformerGetOption) cache.Informer); ok { + r0 = rf(ctx, obj, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Informer) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, client.Object, ...cache.InformerGetOption) error); ok { + r1 = rf(ctx, obj, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetInformerForKind provides a mock function with given fields: ctx, gvk, opts +func (_m *Cache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...cache.InformerGetOption) (cache.Informer, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, gvk) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for GetInformerForKind") + } + + var r0 cache.Informer + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, schema.GroupVersionKind, ...cache.InformerGetOption) (cache.Informer, error)); ok { + return rf(ctx, gvk, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, schema.GroupVersionKind, ...cache.InformerGetOption) cache.Informer); ok { + r0 = rf(ctx, gvk, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Informer) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, schema.GroupVersionKind, ...cache.InformerGetOption) error); ok { + r1 = rf(ctx, gvk, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// IndexField provides a mock function with given fields: ctx, obj, field, extractValue +func (_m *Cache) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error { + ret := _m.Called(ctx, obj, field, extractValue) + + if len(ret) == 0 { + panic("no return value specified for IndexField") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, client.Object, string, client.IndexerFunc) error); ok { + r0 = rf(ctx, obj, field, extractValue) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// List provides a mock function with given fields: ctx, list, opts +func (_m *Cache) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, list) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for List") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, client.ObjectList, ...client.ListOption) error); ok { + r0 = rf(ctx, list, opts...) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// RemoveInformer provides a mock function with given fields: ctx, obj +func (_m *Cache) RemoveInformer(ctx context.Context, obj client.Object) error { + ret := _m.Called(ctx, obj) + + if len(ret) == 0 { + panic("no return value specified for RemoveInformer") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, client.Object) error); ok { + r0 = rf(ctx, obj) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Start provides a mock function with given fields: ctx +func (_m *Cache) Start(ctx context.Context) error { + ret := _m.Called(ctx) + + if len(ret) == 0 { + panic("no return value specified for Start") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context) error); ok { + r0 = rf(ctx) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// WaitForCacheSync provides a mock function with given fields: ctx +func (_m *Cache) WaitForCacheSync(ctx context.Context) bool { + ret := _m.Called(ctx) + + if len(ret) == 0 { + panic("no return value specified for WaitForCacheSync") + } + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context) bool); ok { + r0 = rf(ctx) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// NewCache creates a new instance of Cache. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewCache(t interface { + mock.TestingT + Cleanup(func()) +}) *Cache { + mock := &Cache{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/mocks/controller-runtime/pkg/cache/new_cache_func.go b/mocks/controller-runtime/pkg/cache/new_cache_func.go new file mode 100644 index 00000000..502380e9 --- /dev/null +++ b/mocks/controller-runtime/pkg/cache/new_cache_func.go @@ -0,0 +1,47 @@ +// Code generated by mockery v2.53.3. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + cache "sigs.k8s.io/controller-runtime/pkg/cache" +) + +// newCacheFunc is an autogenerated mock type for the newCacheFunc type +type newCacheFunc struct { + mock.Mock +} + +// Execute provides a mock function with given fields: config, namespace +func (_m *newCacheFunc) Execute(config cache.Config, namespace string) cache.Cache { + ret := _m.Called(config, namespace) + + if len(ret) == 0 { + panic("no return value specified for Execute") + } + + var r0 cache.Cache + if rf, ok := ret.Get(0).(func(cache.Config, string) cache.Cache); ok { + r0 = rf(config, namespace) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Cache) + } + } + + return r0 +} + +// newNewCacheFunc creates a new instance of newCacheFunc. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func newNewCacheFunc(t interface { + mock.TestingT + Cleanup(func()) +}) *newCacheFunc { + mock := &newCacheFunc{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go index 279de76f..de1309ea 100644 --- a/pkg/runtime/adoption_reconciler.go +++ b/pkg/runtime/adoption_reconciler.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" k8sctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -57,7 +58,7 @@ type adoptionReconciler struct { // of an upstream controller-runtime.Manager func (r *adoptionReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() + r.ackResourceCache = mgr.GetCache() return ctrlrt.NewControllerManagedBy( mgr, ).For( @@ -251,7 +252,7 @@ func (r *adoptionReconciler) Sync( // Only create the described resource if it does not already exist // in k8s cluster. - if err := r.apiReader.Get(ctx, types.NamespacedName{ + if err := r.ackResourceCache.Get(ctx, types.NamespacedName{ Namespace: described.MetaObject().GetNamespace(), Name: described.MetaObject().GetName(), }, described.RuntimeObject()); err != nil { @@ -306,7 +307,7 @@ func (r *adoptionReconciler) getAdoptedResource( req ctrlrt.Request, ) (*ackv1alpha1.AdoptedResource, error) { ro := &ackv1alpha1.AdoptedResource{} - // Here we use k8s APIReader to read the k8s object by making the + // Here we use k8s ACKResourceCache to read the k8s object by making the // direct call to k8s apiserver instead of using k8sClient. // The reason is that k8sClient uses a cache and sometimes k8sClient can // return stale copy of object. @@ -314,7 +315,7 @@ func (r *adoptionReconciler) getAdoptedResource( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.apiReader.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -615,7 +616,7 @@ func NewAdoptionReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, + ackResourceCache cache.Cache, ) acktypes.AdoptedResourceReconciler { return &adoptionReconciler{ reconciler: reconciler{ @@ -625,7 +626,7 @@ func NewAdoptionReconcilerWithClient( metrics: metrics, cache: cache, kc: kc, - apiReader: apiReader, + ackResourceCache: ackResourceCache, }, } } diff --git a/pkg/runtime/adoption_reconciler_test.go b/pkg/runtime/adoption_reconciler_test.go index 2c21fd16..e241cada 100644 --- a/pkg/runtime/adoption_reconciler_test.go +++ b/pkg/runtime/adoption_reconciler_test.go @@ -29,6 +29,7 @@ import ( ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" + ctrlrtcachemock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/cache" ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" @@ -45,7 +46,7 @@ const ( // Helper functions for tests -func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -60,7 +61,7 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - apiReader := &ctrlrtclientmock.Reader{} + ackResourceCache := &ctrlrtcachemock.Cache{} return ackrt.NewAdoptionReconcilerWithClient( sc, fakeLogger, @@ -68,8 +69,8 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient metrics, ackrtcache.Caches{}, kc, - apiReader, - ), kc, apiReader + ackResourceCache, + ), kc, ackResourceCache } func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource, *ackmocks.AWSResource) { @@ -126,8 +127,8 @@ func setupMockDescriptor(descriptor *ackmocks.AWSResourceDescriptor, res *ackmoc descriptor.On("MarkAdopted", res).Run(func(args mock.Arguments) {}) } -func setupMockApiReaderForAdoptedResource(apiReader *ctrlrtclientmock.Reader, ctx context.Context, res *ackmocks.AWSResource) { - apiReader.On("Get", ctx, types.NamespacedName{ +func setupMockACKResourceCacheForAdoptedResource(ackResourceCache *ctrlrtcachemock.Cache, ctx context.Context, res *ackmocks.AWSResource) { + ackResourceCache.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(k8serrors.NewNotFound(schema.GroupResource{}, "")) @@ -154,7 +155,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourcesCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -179,7 +180,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { // of SetIdentifiers failure manager.AssertNotCalled(t, "ReadOne", ctx, res) // No calls to findout if the AWSResource already exists - apiReader.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + ackResourcesCache.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -192,7 +193,7 @@ func TestSync_FailureInReadOne(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -216,7 +217,7 @@ func TestSync_FailureInReadOne(t *testing.T) { manager.AssertCalled(t, "ReadOne", ctx, res) // No calls to findout if the AWSResource already exists because of ReadOne // failure - apiReader.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + ackResourceCache.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -229,7 +230,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -242,7 +243,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - apiReader.On("Get", ctx, types.NamespacedName{ + ackResourceCache.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(nil) @@ -252,17 +253,17 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) } -func TestSync_APIReaderUnknownError(t *testing.T) { +func TestSync_ACKResourceCacheUnknownError(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -275,7 +276,7 @@ func TestSync_APIReaderUnknownError(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - apiReader.On("Get", ctx, types.NamespacedName{ + ackResourceCache.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(errors.New("unknown error")) @@ -286,7 +287,7 @@ func TestSync_APIReaderUnknownError(t *testing.T) { //Assertions require.NotNil(err) require.Equal("unknown error", err.Error()) - assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) @@ -296,7 +297,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -308,7 +309,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, ctx, res) + setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(errors.New("creation failure")) // Call @@ -317,7 +318,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { //Assertions require.NotNil(err) require.Equal("creation failure", err.Error()) - assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) kc.AssertCalled(t, "Create", ctx, res.RuntimeObject()) // Update status of AWSResource should not happen due to creation failure statusWriter.AssertNotCalled(t, "Update", ctx, res.RuntimeObject()) @@ -329,7 +330,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -341,7 +342,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, ctx, res) + setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(nil) statusWriter.On("Update", ctx, res.RuntimeObject()).Return(errors.New("status update failure")) @@ -351,7 +352,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { //Assertions require.NotNil(err) require.Equal("status update failure", err.Error()) - assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) @@ -361,7 +362,7 @@ func TestSync_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc, ackResourceCache := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -373,7 +374,7 @@ func TestSync_HappyCase(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, ctx, res) + setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(nil) statusWriter.On("Update", ctx, res.RuntimeObject()).Return(nil) @@ -382,7 +383,7 @@ func TestSync_HappyCase(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) @@ -456,19 +457,19 @@ func assertAWSResourceCreation( // assertAWSResourceRead asserts that // a) Identifiers are set from AdoptedResource to AWSResource // b) ReadOne call is made to find observed state of AWSResource -// c) APIReader.Get call is made to validate that AWSResource does not already +// c) ACKResourceCache.Get call is made to validate that AWSResource does not already // exist in k8s cluster func assertAWSResourceRead( t *testing.T, ctx context.Context, manager *ackmocks.AWSResourceManager, - apiReader *ctrlrtclientmock.Reader, + ackResourceCache *ctrlrtcachemock.Cache, adoptedRes *ackv1alpha1.AdoptedResource, res *ackmocks.AWSResource, ) { res.AssertCalled(t, "SetIdentifiers", adoptedRes.Spec.AWS) manager.AssertCalled(t, "ReadOne", ctx, res) - apiReader.AssertCalled(t, "Get", ctx, types.NamespacedName{ + ackResourceCache.AssertCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go index 19e8ebd4..2b845840 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" k8sctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -63,7 +64,7 @@ type fieldExportReconciler struct { // CRs func (r *fieldExportReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() + r.ackResourceCache = mgr.GetCache() return ctrlrt.NewControllerManagedBy( mgr, @@ -197,7 +198,7 @@ func (r *fieldExportReconciler) getFieldExport( req ctrlrt.Request, ) (*ackv1alpha1.FieldExport, error) { ro := &ackv1alpha1.FieldExport{} - // Here we use k8s APIReader to read the k8s object by making the + // Here we use k8s ackResourceCache to read the k8s object by making the // direct call to k8s apiserver instead of using k8sClient. // The reason is that k8sClient uses a cache and sometimes k8sClient can // return stale copy of object. @@ -205,7 +206,7 @@ func (r *fieldExportReconciler) getFieldExport( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.apiReader.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -219,7 +220,7 @@ func (r *fieldExportReconciler) getSourceResource( name types.NamespacedName, ) (acktypes.AWSResource, error) { obj := rd.EmptyRuntimeObject() - if err := r.apiReader.Get(ctx, name, obj); err != nil { + if err := r.ackResourceCache.Get(ctx, name, obj); err != nil { return nil, err } res := rd.ResourceFromRuntimeObject(obj) @@ -311,7 +312,7 @@ func (r *fieldExportReconciler) writeToConfigMap( } cm := &corev1.ConfigMap{} - err := r.apiReader.Get(ctx, nsn, cm) + err := r.ackResourceCache.Get(ctx, nsn, cm) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingConfigMap.Error()) } @@ -358,7 +359,7 @@ func (r *fieldExportReconciler) writeToSecret( } secret := &corev1.Secret{} - err := r.apiReader.Get(ctx, nsn, secret) + err := r.ackResourceCache.Get(ctx, nsn, secret) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingSecret.Error()) } @@ -389,7 +390,7 @@ func (r *fieldExportReconciler) GetFieldExportsForResource( opts := []client.ListOption{ client.InNamespace(nsn.Namespace), } - if err := r.apiReader.List(ctx, listed, opts...); err != nil { + if err := r.ackResourceCache.List(ctx, listed, opts...); err != nil { return []ackv1alpha1.FieldExport{}, err } @@ -638,7 +639,7 @@ type fieldExportResourceReconciler struct { // CRs func (r *fieldExportResourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() + r.ackResourceCache = mgr.GetCache() if ackcompare.IsNil(r.rd) { return errors.New("cannot bind to AWS resource. reconciler marked for reconciling field exports") @@ -731,17 +732,17 @@ func NewFieldExportReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, + ackResourceCache cache.Cache, ) acktypes.FieldExportReconciler { return &fieldExportReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("field-export-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, - apiReader: apiReader, + sc: sc, + log: log.WithName("field-export-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, + ackResourceCache: ackResourceCache, }, } } @@ -758,19 +759,19 @@ func NewFieldExportResourceReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, + ackResourceCache cache.Cache, rd acktypes.AWSResourceDescriptor, ) acktypes.FieldExportReconciler { return &fieldExportResourceReconciler{ fieldExportReconciler: fieldExportReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("field-export-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, - apiReader: apiReader, + sc: sc, + log: log.WithName("field-export-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, + ackResourceCache: ackResourceCache, }, }, rd: rd, diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index f2d30a31..4408a2b5 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -42,6 +42,7 @@ import ( apimachineryruntimemock "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime" k8srtschemamocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema" ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" + ctrlrtcachemock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/cache" mocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) @@ -61,11 +62,11 @@ var ( // Helper functions for tests -func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { return mockFieldExportReconcilerWithResourceDescriptor(mockResourceDescriptor()) } -func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -80,7 +81,7 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - apiReader := &ctrlrtclientmock.Reader{} + ackResourceCache := &ctrlrtcachemock.Cache{} return ackrt.NewFieldExportReconcilerWithClient( sc, fakeLogger, @@ -88,8 +89,8 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri metrics, ackrtcache.Caches{}, kc, - apiReader, - ), kc, apiReader + ackResourceCache, + ), kc, ackResourceCache } func mockResourceDescriptor() *mocks.AWSResourceDescriptor { @@ -112,12 +113,12 @@ func setupMockClientForFieldExport(kc *ctrlrtclientmock.Client, statusWriter *ct kc.On("Patch", withoutCancelContextMatcher, mock.Anything, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) } -func setupMockApiReaderForFieldExport(apiReader *ctrlrtclientmock.Reader, ctx context.Context, res *mocks.AWSResource) { - apiReader.On("Get", ctx, types.NamespacedName{ +func setupMockACKResourceCacheForFieldExport(ackResourceCache *ctrlrtcachemock.Cache, ctx context.Context, res *mocks.AWSResource) { + ackResourceCache.On("Get", ctx, types.NamespacedName{ Namespace: FieldExportNamespace, Name: "fake-export-output", }, mock.AnythingOfType("*v1.ConfigMap")).Return(nil) - apiReader.On("Get", ctx, types.NamespacedName{ + ackResourceCache.On("Get", ctx, types.NamespacedName{ Namespace: FieldExportNamespace, Name: "fake-export-output", }, mock.AnythingOfType("*v1.Secret")).Return(nil) @@ -267,7 +268,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, "bad-query") @@ -277,7 +278,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -297,7 +298,7 @@ func TestSync_FailureInGetField(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, ".doesnt.exist") @@ -307,7 +308,7 @@ func TestSync_FailureInGetField(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -327,7 +328,7 @@ func TestSync_FailureInPatchConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -339,7 +340,7 @@ func TestSync_FailureInPatchConfigMap(t *testing.T) { kc.On("Patch", withoutCancelContextMatcher, mock.AnythingOfType("*v1.ConfigMap"), mock.AnythingOfType("*client.mergeFromPatch")).Return(errors.New("patching denied")) setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -359,7 +360,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -369,7 +370,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -389,7 +390,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportSecret(FieldExportNamespace, FieldExportName) @@ -399,7 +400,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -419,10 +420,10 @@ func TestFilterAllExports_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, apiReader := mockFieldExportReconciler() + r, _, ackResourceCache := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - apiReader.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). + ackResourceCache.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). Run(func(args mock.Arguments) { // Replace the field export list argument pointer with our mocks list := args.Get(1).(*ackv1alpha1.FieldExportList) @@ -452,11 +453,11 @@ func TestSync_HappyCaseResourceNoExports(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, apiReader := mockFieldExportReconciler() + r, _, ackResourceCache := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - apiReader.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). + ackResourceCache.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). Run(func(args mock.Arguments) { // Replace the field export list argument pointer with our mocks list := args.Get(1).(*ackv1alpha1.FieldExportList) @@ -486,7 +487,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "new-key") @@ -496,7 +497,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -516,7 +517,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc, ackResourceCache := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "") @@ -526,7 +527,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index ddc94f46..27966008 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" ctrlrtcontroller "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -61,13 +62,13 @@ const ( // reconciler describes a generic reconciler within ACK. type reconciler struct { - sc acktypes.ServiceController - kc client.Client - apiReader client.Reader - log logr.Logger - cfg ackcfg.Config - cache ackrtcache.Caches - metrics *ackmetrics.Metrics + sc acktypes.ServiceController + kc client.Client + log logr.Logger + cfg ackcfg.Config + cache ackrtcache.Caches + metrics *ackmetrics.Metrics + ackResourceCache cache.Cache } // resourceReconciler is responsible for reconciling the state of a SINGLE KIND of @@ -101,7 +102,7 @@ func (r *resourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { return ackerr.NilResourceManagerFactory } r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() + r.ackResourceCache = mgr.GetCache() rd := r.rmf.ResourceDescriptor() maxConcurrentReconciles := r.cfg.GetReconcileResourceMaxConcurrency(rd.GroupVersionKind().Kind) return ctrlrt.NewControllerManagedBy( @@ -148,7 +149,7 @@ func (r *reconciler) SecretValueFromReference( Name: ref.Name, } var secret corev1.Secret - if err := r.apiReader.Get(ctx, nsn, &secret); err != nil { + if err := r.ackResourceCache.Get(ctx, nsn, &secret); err != nil { return "", ackerr.SecretNotFound } @@ -182,7 +183,7 @@ func (r *reconciler) WriteToSecret( nsn.Namespace = namespace secret := &corev1.Secret{} - err := r.apiReader.Get(ctx, nsn, secret) + err := r.ackResourceCache.Get(ctx, nsn, secret) if err != nil { return ackerr.SecretNotFound } @@ -363,7 +364,7 @@ func (r *resourceReconciler) reconcile( !(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) && IsReadOnly(res)) { // Resolve references before deleting the resource. // Ignore any errors while resolving the references - resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, res) + resolved, _, _ := rm.ResolveReferences(ctx, r.ackResourceCache, res) return r.deleteResource(ctx, rm, resolved) } @@ -425,7 +426,7 @@ func (r *resourceReconciler) Sync( } rlog.Enter("rm.ResolveReferences") - resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired) + resolved, hasReferences, err := rm.ResolveReferences(ctx, r.ackResourceCache, desired) rlog.Exit("rm.ResolveReferences", err) // TODO (michaelhtm): should we fail here for `adopt-or-create` adoption policy? if err != nil && !needAdoption && !isReadOnly { @@ -1094,7 +1095,7 @@ func (r *resourceReconciler) getAWSResource( req ctrlrt.Request, ) (acktypes.AWSResource, error) { ro := r.rd.EmptyRuntimeObject() - // Here we use k8s APIReader to read the k8s object by making the + // Here we use k8s ackResourceCache to read the k8s object by making the // direct call to k8s apiserver instead of using k8sClient. // The reason is that k8sClient uses a cache and sometimes k8sClient can // return stale copy of object. @@ -1102,7 +1103,7 @@ func (r *resourceReconciler) getAWSResource( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.apiReader.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return r.rd.ResourceFromRuntimeObject(ro), nil diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index fb051543..84a1945d 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -505,7 +505,7 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) { latest, latestRTObj, latestMetaObj := resourceMocks() latest.On("Identifiers").Return(ids) latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) - latest.On( + latest.On( "ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition"), ).Return().Run(func(args mock.Arguments) { From 54860f1f21e80e5b41f47de6ff8c99aafd5243ce Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Thu, 17 Jul 2025 13:15:07 -0700 Subject: [PATCH 2/4] gofmt all files --- apis/core/v1alpha1/annotations.go | 2 +- pkg/condition/condition.go | 2 +- pkg/runtime/adoption_reconciler.go | 12 ++++++------ pkg/runtime/field_export_reconciler_test.go | 4 ++-- pkg/runtime/service_controller.go | 6 +++--- pkg/runtime/util_test.go | 2 +- pkg/types/aws_resource.go | 2 +- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/apis/core/v1alpha1/annotations.go b/apis/core/v1alpha1/annotations.go index 27c76732..4b4c40e6 100644 --- a/apis/core/v1alpha1/annotations.go +++ b/apis/core/v1alpha1/annotations.go @@ -82,7 +82,7 @@ const ( // ACK service controller. AnnotationReadOnly = AnnotationPrefix + "read-only" // AnnotationAdoptionPolicy is an annotation whose value is the identifier for whether - // we will attempt adoption only (value = adopt-only) or attempt a create if resource + // we will attempt adoption only (value = adopt-only) or attempt a create if resource // is not found (value adopt-or-create). // // NOTE (michaelhtm): Currently create-or-adopt is not supported diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index 208508a1..132d1b82 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -29,7 +29,7 @@ var ( NotManagedReason = "This resource already exists but is not managed by ACK. " + "To bring the resource under ACK management, you should explicitly adopt " + "the resource by enabling the ResourceAdoption feature gate and populating " + - "the `services.k8s.aws/adoption-policy` and `services.k8s.aws/adoption-fields` " + + "the `services.k8s.aws/adoption-policy` and `services.k8s.aws/adoption-fields` " + "annotations." UnknownSyncedMessage = "Unable to determine if desired resource state matches latest observed state" NotSyncedMessage = "Resource not synced" diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go index de1309ea..3ddf7f1f 100644 --- a/pkg/runtime/adoption_reconciler.go +++ b/pkg/runtime/adoption_reconciler.go @@ -620,12 +620,12 @@ func NewAdoptionReconcilerWithClient( ) acktypes.AdoptedResourceReconciler { return &adoptionReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("adopted-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, + sc: sc, + log: log.WithName("adopted-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, ackResourceCache: ackResourceCache, }, } diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index 4408a2b5..15880c3e 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -41,8 +41,8 @@ import ( apimachineryruntimemock "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime" k8srtschemamocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema" - ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" ctrlrtcachemock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/cache" + ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" mocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) @@ -595,7 +595,7 @@ func assertPatchedSecretWithKey(expected bool, t *testing.T, ctx context.Context return bytes.Equal(val, []byte("test-book-name")) }) if expected { - kc.AssertCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) + kc.AssertCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) } else { kc.AssertNotCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) } diff --git a/pkg/runtime/service_controller.go b/pkg/runtime/service_controller.go index 7488c127..9fb2ce04 100644 --- a/pkg/runtime/service_controller.go +++ b/pkg/runtime/service_controller.go @@ -331,9 +331,9 @@ func NewServiceController( ) acktypes.ServiceController { return &serviceController{ ServiceControllerMetadata: acktypes.ServiceControllerMetadata{ - VersionInfo: versionInfo, - ServiceAlias: svcAlias, - ServiceAPIGroup: svcAPIGroup, + VersionInfo: versionInfo, + ServiceAlias: svcAlias, + ServiceAPIGroup: svcAPIGroup, }, metrics: ackmetrics.NewMetrics(svcAlias), } diff --git a/pkg/runtime/util_test.go b/pkg/runtime/util_test.go index 74589e3a..a03b2f33 100644 --- a/pkg/runtime/util_test.go +++ b/pkg/runtime/util_test.go @@ -92,7 +92,7 @@ func TestIsForcedAdoption(t *testing.T) { res = &mocks.AWSResource{} res.On("MetaObject").Return(&metav1.ObjectMeta{ Annotations: map[string]string{ - ackv1alpha1.AnnotationAdopted: "true", + ackv1alpha1.AnnotationAdopted: "true", }, }) require.False(ackrt.NeedAdoption(res)) diff --git a/pkg/types/aws_resource.go b/pkg/types/aws_resource.go index dd2673ea..6e9bbc34 100644 --- a/pkg/types/aws_resource.go +++ b/pkg/types/aws_resource.go @@ -50,5 +50,5 @@ type AWSResource interface { DeepCopy() AWSResource // PopulateResourceFromAnnotation will set the Spec or Status field that user // provided from annotations - PopulateResourceFromAnnotation(fields map[string]string) error + PopulateResourceFromAnnotation(fields map[string]string) error } From f6e2d30e0d6738c76be3a9e5c64f6bfafa8499d9 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Wed, 6 Aug 2025 15:01:34 -0700 Subject: [PATCH 3/4] Add ObservedGeneration to ACK Status Conditions Observed Generation is a kubernetes standard that persists the latest reconciled generation (metadata.generation) in the resource status condition --- apis/core/v1alpha1/conditions.go | 5 +++++ pkg/condition/condition.go | 15 ++++++++++----- pkg/condition/condition_test.go | 22 +++++++++++++++++++++- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/apis/core/v1alpha1/conditions.go b/apis/core/v1alpha1/conditions.go index 49e62cc0..df253f9a 100644 --- a/apis/core/v1alpha1/conditions.go +++ b/apis/core/v1alpha1/conditions.go @@ -72,6 +72,11 @@ type Condition struct { Type ConditionType `json:"type"` // Status of the condition, one of True, False, Unknown. Status corev1.ConditionStatus `json:"status"` + // observedGeneration represents the .metadata.generation that the condition was set based upon. + // For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration + // is 9, the condition is out of date with respect to the current state of the instance. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` // Last time the condition transitioned from one status to another. // +optional LastTransitionTime *metav1.Time `json:"lastTransitionTime,omitempty"` diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index 132d1b82..564b0cd9 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -105,7 +105,7 @@ func AllOfType( // SetSynced sets the resource's Condition of type ConditionTypeResourceSynced // to the supplied status, optional message and reason. func SetSynced( - subject acktypes.ConditionManager, + subject acktypes.AWSResource, status corev1.ConditionStatus, message *string, reason *string, @@ -121,6 +121,7 @@ func SetSynced( now := metav1.Now() c.LastTransitionTime = &now c.Status = status + c.ObservedGeneration = subject.MetaObject().GetGeneration() c.Message = message c.Reason = reason subject.ReplaceConditions(allConds) @@ -129,7 +130,7 @@ func SetSynced( // SetTerminal sets the resource's Condition of type ConditionTypeTerminal to // the supplied status, optional message and reason. func SetTerminal( - subject acktypes.ConditionManager, + subject acktypes.AWSResource, status corev1.ConditionStatus, message *string, reason *string, @@ -145,6 +146,7 @@ func SetTerminal( now := metav1.Now() c.LastTransitionTime = &now c.Status = status + c.ObservedGeneration = subject.MetaObject().GetGeneration() c.Message = message c.Reason = reason subject.ReplaceConditions(allConds) @@ -153,7 +155,7 @@ func SetTerminal( // SetRecoverable sets the resource's Condition of type ConditionTypeRecoverable // to the supplied status, optional message and reason. func SetRecoverable( - subject acktypes.ConditionManager, + subject acktypes.AWSResource, status corev1.ConditionStatus, message *string, reason *string, @@ -169,6 +171,7 @@ func SetRecoverable( now := metav1.Now() c.LastTransitionTime = &now c.Status = status + c.ObservedGeneration = subject.MetaObject().GetGeneration() c.Message = message c.Reason = reason subject.ReplaceConditions(allConds) @@ -177,7 +180,7 @@ func SetRecoverable( // SetLateInitialized sets the resource's Condition of type ConditionTypeLateInitialized to // the supplied status, optional message and reason. func SetLateInitialized( - subject acktypes.ConditionManager, + subject acktypes.AWSResource, status corev1.ConditionStatus, message *string, reason *string, @@ -193,6 +196,7 @@ func SetLateInitialized( now := metav1.Now() c.LastTransitionTime = &now c.Status = status + c.ObservedGeneration = subject.MetaObject().GetGeneration() c.Message = message c.Reason = reason subject.ReplaceConditions(allConds) @@ -201,7 +205,7 @@ func SetLateInitialized( // SetReferencesResolved sets the resource's Condition of type ConditionTypeReferencesResolved // to the supplied status, optional message and reason. func SetReferencesResolved( - subject acktypes.ConditionManager, + subject acktypes.AWSResource, status corev1.ConditionStatus, message *string, reason *string, @@ -217,6 +221,7 @@ func SetReferencesResolved( now := metav1.Now() c.LastTransitionTime = &now c.Status = status + c.ObservedGeneration = subject.MetaObject().GetGeneration() c.Message = message c.Reason = reason subject.ReplaceConditions(allConds) diff --git a/pkg/condition/condition_test.go b/pkg/condition/condition_test.go index c27786ff..3506c109 100644 --- a/pkg/condition/condition_test.go +++ b/pkg/condition/condition_test.go @@ -25,6 +25,7 @@ import ( ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors" corev1 "k8s.io/api/core/v1" + metav1mocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/apis/meta/v1" ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) @@ -111,7 +112,10 @@ func TestConditionGetters(t *testing.T) { func TestConditionSetters(t *testing.T) { r := &ackmocks.AWSResource{} r.On("Conditions").Return([]*ackv1alpha1.Condition{}) - + metaObject := &metav1mocks.Object{} + var observedGeneration int64 = 1 + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) // Ensure that if there is no synced condition, it gets added... r.On( "ReplaceConditions", @@ -132,6 +136,8 @@ func TestConditionSetters(t *testing.T) { // Ensure that SetSynced doesn't overwrite any other conditions... r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("Conditions").Return( []*ackv1alpha1.Condition{ &ackv1alpha1.Condition{ @@ -157,6 +163,8 @@ func TestConditionSetters(t *testing.T) { // Ensure that SetSynced overwrites an existing synced condition... r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("Conditions").Return( []*ackv1alpha1.Condition{ &ackv1alpha1.Condition{ @@ -183,6 +191,8 @@ func TestConditionSetters(t *testing.T) { // Ensure that if there is no terminal condition, it gets added... r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( "ReplaceConditions", @@ -204,6 +214,8 @@ func TestConditionSetters(t *testing.T) { // ReferencesResolved condition // SetReferencesResolved r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( "ReplaceConditions", @@ -219,6 +231,8 @@ func TestConditionSetters(t *testing.T) { //RemoveReferencesResolved r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("Conditions").Return( []*ackv1alpha1.Condition{ &ackv1alpha1.Condition{ @@ -246,6 +260,8 @@ func TestConditionSetters(t *testing.T) { //WithReferencesResolvedCondition // Without Error r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( @@ -263,6 +279,8 @@ func TestConditionSetters(t *testing.T) { errorMsg := "error message" err := errors.New(errorMsg) r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( @@ -280,6 +298,8 @@ func TestConditionSetters(t *testing.T) { // With Terminal Error terminalError := ackerr.ResourceReferenceTerminal r = &ackmocks.AWSResource{} + r.On("MetaObject").Return(metaObject) + metaObject.On("GetGeneration").Return(observedGeneration) r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( From 73efccc7d7aae86720a2bd1a278757f7f17aebc0 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Wed, 6 Aug 2025 15:57:34 -0700 Subject: [PATCH 4/4] Use controller-runtime Client instead of Cache Using Controller-Runtime Client is safer in general, as it will attempt to read objects from the APIServer if it was not cached in locally. For example, when using resource references, or using ACK to reference secrets, we may need to read these objects from the APIServer. --- pkg/runtime/adoption_reconciler.go | 10 +- pkg/runtime/adoption_reconciler_test.go | 55 ++++---- pkg/runtime/field_export_reconciler.go | 45 +++---- pkg/runtime/field_export_reconciler_test.go | 51 ++++---- pkg/runtime/reconciler.go | 25 ++-- pkg/runtime/reconciler_test.go | 134 ++++++++++---------- 6 files changed, 150 insertions(+), 170 deletions(-) diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go index 3ddf7f1f..0cb2eddd 100644 --- a/pkg/runtime/adoption_reconciler.go +++ b/pkg/runtime/adoption_reconciler.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" k8sctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -58,7 +57,6 @@ type adoptionReconciler struct { // of an upstream controller-runtime.Manager func (r *adoptionReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.ackResourceCache = mgr.GetCache() return ctrlrt.NewControllerManagedBy( mgr, ).For( @@ -252,7 +250,7 @@ func (r *adoptionReconciler) Sync( // Only create the described resource if it does not already exist // in k8s cluster. - if err := r.ackResourceCache.Get(ctx, types.NamespacedName{ + if err := r.kc.Get(ctx, types.NamespacedName{ Namespace: described.MetaObject().GetNamespace(), Name: described.MetaObject().GetName(), }, described.RuntimeObject()); err != nil { @@ -315,7 +313,7 @@ func (r *adoptionReconciler) getAdoptedResource( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.kc.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -602,7 +600,7 @@ func NewAdoptionReconciler( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, ) acktypes.AdoptedResourceReconciler { - return NewAdoptionReconcilerWithClient(sc, log, cfg, metrics, cache, nil, nil) + return NewAdoptionReconcilerWithClient(sc, log, cfg, metrics, cache, nil) } // NewAdoptionReconcilerWithClient returns a new adoptionReconciler object with @@ -616,7 +614,6 @@ func NewAdoptionReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - ackResourceCache cache.Cache, ) acktypes.AdoptedResourceReconciler { return &adoptionReconciler{ reconciler: reconciler{ @@ -626,7 +623,6 @@ func NewAdoptionReconcilerWithClient( metrics: metrics, cache: cache, kc: kc, - ackResourceCache: ackResourceCache, }, } } diff --git a/pkg/runtime/adoption_reconciler_test.go b/pkg/runtime/adoption_reconciler_test.go index e241cada..a9b0e420 100644 --- a/pkg/runtime/adoption_reconciler_test.go +++ b/pkg/runtime/adoption_reconciler_test.go @@ -29,7 +29,6 @@ import ( ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" - ctrlrtcachemock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/cache" ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" @@ -46,7 +45,7 @@ const ( // Helper functions for tests -func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { +func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -61,7 +60,6 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - ackResourceCache := &ctrlrtcachemock.Cache{} return ackrt.NewAdoptionReconcilerWithClient( sc, fakeLogger, @@ -69,8 +67,7 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient metrics, ackrtcache.Caches{}, kc, - ackResourceCache, - ), kc, ackResourceCache + ), kc } func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource, *ackmocks.AWSResource) { @@ -127,8 +124,8 @@ func setupMockDescriptor(descriptor *ackmocks.AWSResourceDescriptor, res *ackmoc descriptor.On("MarkAdopted", res).Run(func(args mock.Arguments) {}) } -func setupMockACKResourceCacheForAdoptedResource(ackResourceCache *ctrlrtcachemock.Cache, ctx context.Context, res *ackmocks.AWSResource) { - ackResourceCache.On("Get", ctx, types.NamespacedName{ +func setupMockACKResourceCacheForAdoptedResource(kc *ctrlrtclientmock.Client, ctx context.Context, res *ackmocks.AWSResource) { + kc.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(k8serrors.NewNotFound(schema.GroupResource{}, "")) @@ -155,7 +152,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourcesCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -180,7 +177,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { // of SetIdentifiers failure manager.AssertNotCalled(t, "ReadOne", ctx, res) // No calls to findout if the AWSResource already exists - ackResourcesCache.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + kc.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -193,7 +190,7 @@ func TestSync_FailureInReadOne(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -217,7 +214,7 @@ func TestSync_FailureInReadOne(t *testing.T) { manager.AssertCalled(t, "ReadOne", ctx, res) // No calls to findout if the AWSResource already exists because of ReadOne // failure - ackResourceCache.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + kc.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -230,7 +227,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -243,7 +240,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - ackResourceCache.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(nil) @@ -253,7 +250,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, kc, adoptedRes, res) assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) @@ -263,7 +260,7 @@ func TestSync_ACKResourceCacheUnknownError(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -276,7 +273,7 @@ func TestSync_ACKResourceCacheUnknownError(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - ackResourceCache.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(errors.New("unknown error")) @@ -287,7 +284,7 @@ func TestSync_ACKResourceCacheUnknownError(t *testing.T) { //Assertions require.NotNil(err) require.Equal("unknown error", err.Error()) - assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, kc, adoptedRes, res) assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) @@ -297,7 +294,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -309,7 +306,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) + setupMockACKResourceCacheForAdoptedResource(kc, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(errors.New("creation failure")) // Call @@ -318,7 +315,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { //Assertions require.NotNil(err) require.Equal("creation failure", err.Error()) - assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, kc, adoptedRes, res) kc.AssertCalled(t, "Create", ctx, res.RuntimeObject()) // Update status of AWSResource should not happen due to creation failure statusWriter.AssertNotCalled(t, "Update", ctx, res.RuntimeObject()) @@ -330,7 +327,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -342,7 +339,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) + setupMockACKResourceCacheForAdoptedResource(kc, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(nil) statusWriter.On("Update", ctx, res.RuntimeObject()).Return(errors.New("status update failure")) @@ -352,7 +349,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { //Assertions require.NotNil(err) require.Equal("status update failure", err.Error()) - assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, kc, adoptedRes, res) assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(false, t, ctx, kc, adoptedRes) assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes) @@ -362,7 +359,7 @@ func TestSync_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -374,7 +371,7 @@ func TestSync_HappyCase(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockACKResourceCacheForAdoptedResource(ackResourceCache, ctx, res) + setupMockACKResourceCacheForAdoptedResource(kc, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(nil) statusWriter.On("Update", ctx, res.RuntimeObject()).Return(nil) @@ -383,7 +380,7 @@ func TestSync_HappyCase(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, ackResourceCache, adoptedRes, res) + assertAWSResourceRead(t, ctx, manager, kc, adoptedRes, res) assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy) assertAdoptedResourceManaged(true, t, ctx, kc, adoptedRes) assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes) @@ -457,19 +454,19 @@ func assertAWSResourceCreation( // assertAWSResourceRead asserts that // a) Identifiers are set from AdoptedResource to AWSResource // b) ReadOne call is made to find observed state of AWSResource -// c) ACKResourceCache.Get call is made to validate that AWSResource does not already +// c) kc.Get call is made to validate that AWSResource does not already // exist in k8s cluster func assertAWSResourceRead( t *testing.T, ctx context.Context, manager *ackmocks.AWSResourceManager, - ackResourceCache *ctrlrtcachemock.Cache, + kc *ctrlrtclientmock.Client, adoptedRes *ackv1alpha1.AdoptedResource, res *ackmocks.AWSResource, ) { res.AssertCalled(t, "SetIdentifiers", adoptedRes.Spec.AWS) manager.AssertCalled(t, "ReadOne", ctx, res) - ackResourceCache.AssertCalled(t, "Get", ctx, types.NamespacedName{ + kc.AssertCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go index 2b845840..a6a7fe19 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" k8sctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -64,7 +63,6 @@ type fieldExportReconciler struct { // CRs func (r *fieldExportReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.ackResourceCache = mgr.GetCache() return ctrlrt.NewControllerManagedBy( mgr, @@ -206,7 +204,7 @@ func (r *fieldExportReconciler) getFieldExport( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.kc.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -220,7 +218,7 @@ func (r *fieldExportReconciler) getSourceResource( name types.NamespacedName, ) (acktypes.AWSResource, error) { obj := rd.EmptyRuntimeObject() - if err := r.ackResourceCache.Get(ctx, name, obj); err != nil { + if err := r.kc.Get(ctx, name, obj); err != nil { return nil, err } res := rd.ResourceFromRuntimeObject(obj) @@ -312,7 +310,7 @@ func (r *fieldExportReconciler) writeToConfigMap( } cm := &corev1.ConfigMap{} - err := r.ackResourceCache.Get(ctx, nsn, cm) + err := r.kc.Get(ctx, nsn, cm) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingConfigMap.Error()) } @@ -359,7 +357,7 @@ func (r *fieldExportReconciler) writeToSecret( } secret := &corev1.Secret{} - err := r.ackResourceCache.Get(ctx, nsn, secret) + err := r.kc.Get(ctx, nsn, secret) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingSecret.Error()) } @@ -390,7 +388,7 @@ func (r *fieldExportReconciler) GetFieldExportsForResource( opts := []client.ListOption{ client.InNamespace(nsn.Namespace), } - if err := r.ackResourceCache.List(ctx, listed, opts...); err != nil { + if err := r.kc.List(ctx, listed, opts...); err != nil { return []ackv1alpha1.FieldExport{}, err } @@ -639,7 +637,6 @@ type fieldExportResourceReconciler struct { // CRs func (r *fieldExportResourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.ackResourceCache = mgr.GetCache() if ackcompare.IsNil(r.rd) { return errors.New("cannot bind to AWS resource. reconciler marked for reconciling field exports") @@ -706,7 +703,7 @@ func NewFieldExportReconcilerForFieldExport( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, ) acktypes.FieldExportReconciler { - return NewFieldExportReconcilerWithClient(sc, log, cfg, metrics, cache, nil, nil) + return NewFieldExportReconcilerWithClient(sc, log, cfg, metrics, cache, nil) } // NewFieldExportReconcilerForAWSResource returns a new FieldExportReconciler object @@ -718,7 +715,7 @@ func NewFieldExportReconcilerForAWSResource( cache ackrtcache.Caches, rd acktypes.AWSResourceDescriptor, ) acktypes.FieldExportReconciler { - return NewFieldExportResourceReconcilerWithClient(sc, log, cfg, metrics, cache, nil, nil, rd) + return NewFieldExportResourceReconcilerWithClient(sc, log, cfg, metrics, cache, nil, rd) } // NewFieldExportReconcilerWithClient returns a new FieldExportReconciler object with @@ -732,17 +729,15 @@ func NewFieldExportReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - ackResourceCache cache.Cache, ) acktypes.FieldExportReconciler { return &fieldExportReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("field-export-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, - ackResourceCache: ackResourceCache, + sc: sc, + log: log.WithName("field-export-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, }, } } @@ -759,19 +754,17 @@ func NewFieldExportResourceReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - ackResourceCache cache.Cache, rd acktypes.AWSResourceDescriptor, ) acktypes.FieldExportReconciler { return &fieldExportResourceReconciler{ fieldExportReconciler: fieldExportReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("field-export-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, - ackResourceCache: ackResourceCache, + sc: sc, + log: log.WithName("field-export-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, }, }, rd: rd, diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index 15880c3e..d9a9785c 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -41,7 +41,6 @@ import ( apimachineryruntimemock "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime" k8srtschemamocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema" - ctrlrtcachemock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/cache" ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" mocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) @@ -62,11 +61,11 @@ var ( // Helper functions for tests -func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { +func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client) { return mockFieldExportReconcilerWithResourceDescriptor(mockResourceDescriptor()) } -func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtcachemock.Cache) { +func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -81,7 +80,6 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - ackResourceCache := &ctrlrtcachemock.Cache{} return ackrt.NewFieldExportReconcilerWithClient( sc, fakeLogger, @@ -89,8 +87,7 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri metrics, ackrtcache.Caches{}, kc, - ackResourceCache, - ), kc, ackResourceCache + ), kc } func mockResourceDescriptor() *mocks.AWSResourceDescriptor { @@ -113,12 +110,12 @@ func setupMockClientForFieldExport(kc *ctrlrtclientmock.Client, statusWriter *ct kc.On("Patch", withoutCancelContextMatcher, mock.Anything, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) } -func setupMockACKResourceCacheForFieldExport(ackResourceCache *ctrlrtcachemock.Cache, ctx context.Context, res *mocks.AWSResource) { - ackResourceCache.On("Get", ctx, types.NamespacedName{ +func setupMockACKResourceCacheForFieldExport(kc *ctrlrtclientmock.Client, ctx context.Context, res *mocks.AWSResource) { + kc.On("Get", ctx, types.NamespacedName{ Namespace: FieldExportNamespace, Name: "fake-export-output", }, mock.AnythingOfType("*v1.ConfigMap")).Return(nil) - ackResourceCache.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: FieldExportNamespace, Name: "fake-export-output", }, mock.AnythingOfType("*v1.Secret")).Return(nil) @@ -268,7 +265,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, "bad-query") @@ -278,7 +275,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -298,7 +295,7 @@ func TestSync_FailureInGetField(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, ".doesnt.exist") @@ -308,7 +305,7 @@ func TestSync_FailureInGetField(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -328,7 +325,7 @@ func TestSync_FailureInPatchConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -340,7 +337,7 @@ func TestSync_FailureInPatchConfigMap(t *testing.T) { kc.On("Patch", withoutCancelContextMatcher, mock.AnythingOfType("*v1.ConfigMap"), mock.AnythingOfType("*client.mergeFromPatch")).Return(errors.New("patching denied")) setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -360,7 +357,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -370,7 +367,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -390,7 +387,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportSecret(FieldExportNamespace, FieldExportName) @@ -400,7 +397,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -420,10 +417,10 @@ func TestFilterAllExports_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - ackResourceCache.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). + kc.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). Run(func(args mock.Arguments) { // Replace the field export list argument pointer with our mocks list := args.Get(1).(*ackv1alpha1.FieldExportList) @@ -453,11 +450,11 @@ func TestSync_HappyCaseResourceNoExports(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - ackResourceCache.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). + kc.On("List", ctx, mock.AnythingOfType("*v1alpha1.FieldExportList"), mock.Anything).Return(nil). Run(func(args mock.Arguments) { // Replace the field export list argument pointer with our mocks list := args.Get(1).(*ackv1alpha1.FieldExportList) @@ -487,7 +484,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "new-key") @@ -497,7 +494,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -517,7 +514,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, ackResourceCache := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "") @@ -527,7 +524,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockACKResourceCacheForFieldExport(ackResourceCache, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 27966008..5a246c23 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrlrt "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" ctrlrtcontroller "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -62,13 +61,12 @@ const ( // reconciler describes a generic reconciler within ACK. type reconciler struct { - sc acktypes.ServiceController - kc client.Client - log logr.Logger - cfg ackcfg.Config - cache ackrtcache.Caches - metrics *ackmetrics.Metrics - ackResourceCache cache.Cache + sc acktypes.ServiceController + kc client.Client + log logr.Logger + cfg ackcfg.Config + cache ackrtcache.Caches + metrics *ackmetrics.Metrics } // resourceReconciler is responsible for reconciling the state of a SINGLE KIND of @@ -102,7 +100,6 @@ func (r *resourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { return ackerr.NilResourceManagerFactory } r.kc = mgr.GetClient() - r.ackResourceCache = mgr.GetCache() rd := r.rmf.ResourceDescriptor() maxConcurrentReconciles := r.cfg.GetReconcileResourceMaxConcurrency(rd.GroupVersionKind().Kind) return ctrlrt.NewControllerManagedBy( @@ -149,7 +146,7 @@ func (r *reconciler) SecretValueFromReference( Name: ref.Name, } var secret corev1.Secret - if err := r.ackResourceCache.Get(ctx, nsn, &secret); err != nil { + if err := r.kc.Get(ctx, nsn, &secret); err != nil { return "", ackerr.SecretNotFound } @@ -183,7 +180,7 @@ func (r *reconciler) WriteToSecret( nsn.Namespace = namespace secret := &corev1.Secret{} - err := r.ackResourceCache.Get(ctx, nsn, secret) + err := r.kc.Get(ctx, nsn, secret) if err != nil { return ackerr.SecretNotFound } @@ -364,7 +361,7 @@ func (r *resourceReconciler) reconcile( !(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) && IsReadOnly(res)) { // Resolve references before deleting the resource. // Ignore any errors while resolving the references - resolved, _, _ := rm.ResolveReferences(ctx, r.ackResourceCache, res) + resolved, _, _ := rm.ResolveReferences(ctx, r.kc, res) return r.deleteResource(ctx, rm, resolved) } @@ -426,7 +423,7 @@ func (r *resourceReconciler) Sync( } rlog.Enter("rm.ResolveReferences") - resolved, hasReferences, err := rm.ResolveReferences(ctx, r.ackResourceCache, desired) + resolved, hasReferences, err := rm.ResolveReferences(ctx, r.kc, desired) rlog.Exit("rm.ResolveReferences", err) // TODO (michaelhtm): should we fail here for `adopt-or-create` adoption policy? if err != nil && !needAdoption && !isReadOnly { @@ -1103,7 +1100,7 @@ func (r *resourceReconciler) getAWSResource( // making single read call for complete reconciler loop. // See following issue for more details: // https://github.com/aws-controllers-k8s/community/issues/894 - if err := r.ackResourceCache.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.kc.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return r.rd.ResourceFromRuntimeObject(ro), nil diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 84a1945d..b7beb4b1 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -191,9 +191,6 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { ).Return() rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -220,6 +217,9 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // Use specific matcher for WithoutCancel context instead of mock.Anything kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) _, err := r.Sync(ctx, rm, desired) require.Nil(err) rm.AssertNumberOfCalls(t, "ReadOne", 6) @@ -255,9 +255,6 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) { ).Return() rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -277,6 +274,9 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) { rd.On("Delta", desired, desired).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // Use specific matcher for WithoutCancel context instead of mock.Anything kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -317,9 +317,6 @@ func TestReconcilerReadOnlyResource(t *testing.T) { ).Return() rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -332,6 +329,9 @@ func TestReconcilerReadOnlyResource(t *testing.T) { rmf, _ := managedResourceManagerFactoryMocks(desired, latest) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) statusWriter := &ctrlrtclientmock.SubResourceWriter{} kc.On("Status").Return(statusWriter) @@ -376,9 +376,6 @@ func TestReconcilerAdoptResource(t *testing.T) { ).Return() desired.On("PopulateResourceFromAnnotation", adoptionFields).Return(nil) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -392,6 +389,9 @@ func TestReconcilerAdoptResource(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("FilterSystemTags", latest).Return() rd.On("MarkAdopted", latest).Return() rm.On("EnsureTags", ctx, desired, scmd).Return(nil) @@ -443,9 +443,6 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) { ).Return() rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -467,6 +464,9 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) statusWriter := &ctrlrtclientmock.SubResourceWriter{} kc.On("Status").Return(statusWriter) @@ -541,9 +541,6 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) { ).Return() rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) desired.On("PopulateResourceFromAnnotation", adoptionFields).Return(nil) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", updated).Return(updated) @@ -576,6 +573,9 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) { rd.On("MarkAdopted", updated).Return().Once() r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) statusWriter := &ctrlrtclientmock.SubResourceWriter{} kc.On("Status").Return(statusWriter) @@ -620,9 +620,6 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testin }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Times(2) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -645,6 +642,9 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testin rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -662,7 +662,7 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testin // Only before the ReadOne call do they need to be resolved, and then the // referenced values are cleared when calling patch so they aren't persisted to etcd. rm.AssertNumberOfCalls(t, "ResolveReferences", 1) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rm.AssertCalled(t, "Create", ctx, desired) // No changes to metadata or spec so Patch on the object shouldn't be done @@ -703,9 +703,6 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Once() rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -726,6 +723,9 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -742,7 +742,7 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. // Make sure references are resolved once for the resource creation when // the resource is already managed rm.AssertNumberOfCalls(t, "ResolveReferences", 1) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rm.AssertCalled(t, "Create", ctx, desired) // No changes to metadata or spec so Patch on the object shouldn't be done @@ -786,9 +786,6 @@ func TestReconcilerUpdate(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ).Once() rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -810,6 +807,9 @@ func TestReconcilerUpdate(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ).Times(2) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -826,7 +826,7 @@ func TestReconcilerUpdate(t *testing.T) { require.Nil(err) // Assert that References are resolved only once during resource update rm.AssertNumberOfCalls(t, "ResolveReferences", 1) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -873,9 +873,6 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -897,6 +894,9 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -911,7 +911,7 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -957,9 +957,6 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -975,6 +972,9 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { rd.On("Delta", latest, latest).Return(delta) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -989,7 +989,7 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) // Update is not called because there is no delta @@ -1036,9 +1036,6 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1054,6 +1051,9 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { rd.On("Delta", latest, latest).Return(delta) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -1068,7 +1068,7 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) // Update is not called because there is no delta @@ -1119,9 +1119,6 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1144,6 +1141,9 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during @@ -1158,7 +1158,7 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -1205,9 +1205,6 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1221,13 +1218,16 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -1285,9 +1285,6 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { ) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1301,13 +1298,16 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -1428,9 +1428,6 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, false, nil, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1452,6 +1449,9 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, false, nil, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -1460,7 +1460,7 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { // Assert the error from late initialization require.NotNil(err) assert.Equal(requeueError, err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -1550,11 +1550,11 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rmf, rd := managerFactoryMocks(desired, latest, false) - r, _, scmd := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) rd.On("IsManaged", desired).Return(false) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) - rm.On("ResolveReferences", ctx, nil, desired).Return( + rm.On("ResolveReferences", ctx, kc, desired).Return( desired, false, nil, ) rm.On("ClearResolvedReferences", desired).Return(desired) @@ -1567,7 +1567,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { _, err := r.Sync(ctx, rm, desired) require.NotNil(err) assert.Equal(ackerr.Terminal, err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertCalled(t, "ReadOne", ctx, desired) rd.AssertNotCalled(t, "Delta", desired, latest) rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta) @@ -1616,9 +1616,6 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, true, resolveReferenceError, - ) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1639,6 +1636,9 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return( + desired, true, resolveReferenceError, + ) rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -1650,7 +1650,7 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.NotNil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertNotCalled(t, "ReadOne", ctx, desired) rd.AssertNotCalled(t, "Delta", desired, latest) rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta) @@ -1703,7 +1703,6 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return(desired, false, nil) rm.On("ClearResolvedReferences", desired).Return(desired) rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( @@ -1724,6 +1723,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) r, kc, scmd := reconcilerMocks(rmf) + rm.On("ResolveReferences", ctx, kc, desired).Return(desired, false, nil) rm.On("EnsureTags", ctx, desired, scmd).Return( ensureControllerTagsError, ) @@ -1737,7 +1737,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.NotNil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertCalled(t, "ResolveReferences", ctx, kc, desired) rm.AssertNotCalled(t, "ReadOne", ctx, desired) rd.AssertNotCalled(t, "Delta", desired, latest) rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta)