diff --git a/Makefile b/Makefile index 88f2fbb..b354d4c 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/apis/core/v1alpha1/annotations.go b/apis/core/v1alpha1/annotations.go index 27c7673..4b4c40e 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/apis/core/v1alpha1/conditions.go b/apis/core/v1alpha1/conditions.go index 49e62cc..df253f9 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/mocks/controller-runtime/pkg/cache/cache.go b/mocks/controller-runtime/pkg/cache/cache.go new file mode 100644 index 0000000..3681484 --- /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 0000000..502380e --- /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/condition/condition.go b/pkg/condition/condition.go index 208508a..564b0cd 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" @@ -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 c27786f..3506c10 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( diff --git a/pkg/runtime/adoption_reconciler.go b/pkg/runtime/adoption_reconciler.go index 279de76..0cb2edd 100644 --- a/pkg/runtime/adoption_reconciler.go +++ b/pkg/runtime/adoption_reconciler.go @@ -57,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.apiReader = mgr.GetAPIReader() return ctrlrt.NewControllerManagedBy( mgr, ).For( @@ -251,7 +250,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.kc.Get(ctx, types.NamespacedName{ Namespace: described.MetaObject().GetNamespace(), Name: described.MetaObject().GetName(), }, described.RuntimeObject()); err != nil { @@ -306,7 +305,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 +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.apiReader.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.kc.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -601,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 @@ -615,17 +614,15 @@ func NewAdoptionReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, ) acktypes.AdoptedResourceReconciler { return &adoptionReconciler{ reconciler: reconciler{ - sc: sc, - log: log.WithName("adopted-reconciler"), - cfg: cfg, - metrics: metrics, - cache: cache, - kc: kc, - apiReader: apiReader, + sc: sc, + log: log.WithName("adopted-reconciler"), + cfg: cfg, + metrics: metrics, + cache: cache, + kc: kc, }, } } diff --git a/pkg/runtime/adoption_reconciler_test.go b/pkg/runtime/adoption_reconciler_test.go index 2c21fd1..a9b0e42 100644 --- a/pkg/runtime/adoption_reconciler_test.go +++ b/pkg/runtime/adoption_reconciler_test.go @@ -45,7 +45,7 @@ const ( // Helper functions for tests -func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Client) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -60,7 +60,6 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - apiReader := &ctrlrtclientmock.Reader{} return ackrt.NewAdoptionReconcilerWithClient( sc, fakeLogger, @@ -68,8 +67,7 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient metrics, ackrtcache.Caches{}, kc, - apiReader, - ), kc, apiReader + ), kc } func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource, *ackmocks.AWSResource) { @@ -126,8 +124,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(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{}, "")) @@ -154,7 +152,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -179,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 - apiReader.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + kc.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -192,7 +190,7 @@ func TestSync_FailureInReadOne(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -216,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 - apiReader.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ + kc.AssertNotCalled(t, "Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()) @@ -229,7 +227,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -242,7 +240,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - apiReader.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(nil) @@ -252,17 +250,17 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, apiReader, 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) } -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 := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -275,7 +273,7 @@ func TestSync_APIReaderUnknownError(t *testing.T) { setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - apiReader.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: AdoptedResourceNamespace, Name: AdoptedResourceName, }, res.RuntimeObject()).Return(errors.New("unknown error")) @@ -286,7 +284,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, 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) @@ -296,7 +294,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -308,7 +306,7 @@ func TestSync_ErrorInResourceCreation(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, ctx, res) + setupMockACKResourceCacheForAdoptedResource(kc, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(errors.New("creation failure")) // Call @@ -317,7 +315,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, 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()) @@ -329,7 +327,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -341,7 +339,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, 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")) @@ -351,7 +349,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, 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) @@ -361,7 +359,7 @@ func TestSync_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockAdoptionReconciler() + r, kc := mockAdoptionReconciler() descriptor, res, resDeepCopy := mockDescriptorAndAWSResource() manager := mockManager() adoptedRes := adoptedResource(AdoptedResourceNamespace, AdoptedResourceName) @@ -373,7 +371,7 @@ func TestSync_HappyCase(t *testing.T) { setupMockClientForAdoptedResource(kc, statusWriter, ctx, adoptedRes) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) - setupMockApiReaderForAdoptedResource(apiReader, ctx, res) + setupMockACKResourceCacheForAdoptedResource(kc, ctx, res) kc.On("Create", ctx, res.RuntimeObject()).Return(nil) statusWriter.On("Update", ctx, res.RuntimeObject()).Return(nil) @@ -382,7 +380,7 @@ func TestSync_HappyCase(t *testing.T) { //Assertions require.Nil(err) - assertAWSResourceRead(t, ctx, manager, apiReader, 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) @@ -456,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) APIReader.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, - apiReader *ctrlrtclientmock.Reader, + kc *ctrlrtclientmock.Client, 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{ + 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 19e8ebd..a6a7fe1 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -63,7 +63,6 @@ type fieldExportReconciler struct { // CRs func (r *fieldExportReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() return ctrlrt.NewControllerManagedBy( mgr, @@ -197,7 +196,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 +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.apiReader.Get(ctx, req.NamespacedName, ro); err != nil { + if err := r.kc.Get(ctx, req.NamespacedName, ro); err != nil { return nil, err } return ro, nil @@ -219,7 +218,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.kc.Get(ctx, name, obj); err != nil { return nil, err } res := rd.ResourceFromRuntimeObject(obj) @@ -311,7 +310,7 @@ func (r *fieldExportReconciler) writeToConfigMap( } cm := &corev1.ConfigMap{} - err := r.apiReader.Get(ctx, nsn, cm) + err := r.kc.Get(ctx, nsn, cm) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingConfigMap.Error()) } @@ -358,7 +357,7 @@ func (r *fieldExportReconciler) writeToSecret( } secret := &corev1.Secret{} - err := r.apiReader.Get(ctx, nsn, secret) + err := r.kc.Get(ctx, nsn, secret) if err != nil { return errors.Wrap(err, ackerr.FieldExportMissingSecret.Error()) } @@ -389,7 +388,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.kc.List(ctx, listed, opts...); err != nil { return []ackv1alpha1.FieldExport{}, err } @@ -638,7 +637,6 @@ type fieldExportResourceReconciler struct { // CRs func (r *fieldExportResourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() if ackcompare.IsNil(r.rd) { return errors.New("cannot bind to AWS resource. reconciler marked for reconciling field exports") @@ -705,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 @@ -717,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 @@ -731,17 +729,15 @@ func NewFieldExportReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, ) 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, }, } } @@ -758,19 +754,17 @@ func NewFieldExportResourceReconcilerWithClient( metrics *ackmetrics.Metrics, cache ackrtcache.Caches, kc client.Client, - apiReader client.Reader, 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, }, }, rd: rd, diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index f2d30a3..d9a9785 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -61,11 +61,11 @@ var ( // Helper functions for tests -func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockFieldExportReconciler() (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client) { return mockFieldExportReconcilerWithResourceDescriptor(mockResourceDescriptor()) } -func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client, *ctrlrtclientmock.Reader) { +func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescriptor) (acktypes.FieldExportReconciler, *ctrlrtclientmock.Client) { zapOptions := ctrlrtzap.Options{ Development: true, Level: zapcore.InfoLevel, @@ -80,7 +80,6 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri rmFactoryMap["services.k8s.aws"] = &rmfactory sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} - apiReader := &ctrlrtclientmock.Reader{} return ackrt.NewFieldExportReconcilerWithClient( sc, fakeLogger, @@ -88,8 +87,7 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri metrics, ackrtcache.Caches{}, kc, - apiReader, - ), kc, apiReader + ), kc } func mockResourceDescriptor() *mocks.AWSResourceDescriptor { @@ -112,12 +110,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(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) - apiReader.On("Get", ctx, types.NamespacedName{ + kc.On("Get", ctx, types.NamespacedName{ Namespace: FieldExportNamespace, Name: "fake-export-output", }, mock.AnythingOfType("*v1.Secret")).Return(nil) @@ -267,7 +265,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, "bad-query") @@ -277,7 +275,7 @@ func TestSync_FailureInParsingQuery(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -297,7 +295,7 @@ func TestSync_FailureInGetField(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithPath(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeConfigMap, ".doesnt.exist") @@ -307,7 +305,7 @@ func TestSync_FailureInGetField(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -327,7 +325,7 @@ func TestSync_FailureInPatchConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -339,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) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -359,7 +357,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportConfigMap(FieldExportNamespace, FieldExportName) @@ -369,7 +367,7 @@ func TestSync_HappyCaseConfigMap(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -389,7 +387,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportSecret(FieldExportNamespace, FieldExportName) @@ -399,7 +397,7 @@ func TestSync_HappyCaseSecret(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -419,10 +417,10 @@ func TestFilterAllExports_HappyCase(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - apiReader.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) @@ -452,11 +450,11 @@ func TestSync_HappyCaseResourceNoExports(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, _, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() ctx := context.TODO() mockExports := mockFieldExportList() - apiReader.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) @@ -486,7 +484,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "new-key") @@ -496,7 +494,7 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -516,7 +514,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { // Setup require := require.New(t) // Mock resource creation - r, kc, apiReader := mockFieldExportReconciler() + r, kc := mockFieldExportReconciler() descriptor, res, _ := mockDescriptorAndAWSResource() manager := mockManager() fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "") @@ -526,7 +524,7 @@ func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { //Mock behavior setup setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) - setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockACKResourceCacheForFieldExport(kc, ctx, res) setupMockManager(manager, ctx, res) setupMockDescriptor(descriptor, res) setupMockUnstructuredConverter() @@ -594,7 +592,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/reconciler.go b/pkg/runtime/reconciler.go index ddc94f4..5a246c2 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -61,13 +61,12 @@ 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 } // resourceReconciler is responsible for reconciling the state of a SINGLE KIND of @@ -101,7 +100,6 @@ func (r *resourceReconciler) BindControllerManager(mgr ctrlrt.Manager) error { return ackerr.NilResourceManagerFactory } r.kc = mgr.GetClient() - r.apiReader = mgr.GetAPIReader() rd := r.rmf.ResourceDescriptor() maxConcurrentReconciles := r.cfg.GetReconcileResourceMaxConcurrency(rd.GroupVersionKind().Kind) return ctrlrt.NewControllerManagedBy( @@ -148,7 +146,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.kc.Get(ctx, nsn, &secret); err != nil { return "", ackerr.SecretNotFound } @@ -182,7 +180,7 @@ func (r *reconciler) WriteToSecret( nsn.Namespace = namespace secret := &corev1.Secret{} - err := r.apiReader.Get(ctx, nsn, secret) + err := r.kc.Get(ctx, nsn, secret) if err != nil { return ackerr.SecretNotFound } @@ -363,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.apiReader, res) + resolved, _, _ := rm.ResolveReferences(ctx, r.kc, res) return r.deleteResource(ctx, rm, resolved) } @@ -425,7 +423,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.kc, 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 +1092,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 +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.apiReader.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 fb05154..b7beb4b 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) @@ -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) { @@ -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) diff --git a/pkg/runtime/service_controller.go b/pkg/runtime/service_controller.go index 7488c12..9fb2ce0 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 74589e3..a03b2f3 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 dd2673e..6e9bbc3 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 }