From 46840420c91a8fe905720936ab6d020066a1317f Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Mon, 13 Mar 2023 21:47:12 +0000 Subject: [PATCH 1/7] Add interfaces for separate reference resolution --- pkg/runtime/reconciler.go | 95 ++++++++++++------------- pkg/types/aws_resource_manager.go | 10 +-- pkg/types/resolved_reference_manager.go | 38 ++++++++++ 3 files changed, 85 insertions(+), 58 deletions(-) create mode 100644 pkg/types/resolved_reference_manager.go diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index e3cb7e4..cf08c44 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -201,13 +201,13 @@ func (r *resourceReconciler) reconcile( if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete { // Resolve references before deleting the resource. // Ignore any errors while resolving the references - res, _ = rm.ResolveReferences(ctx, r.apiReader, res) + _ = rm.ResolveReferences(ctx, r.apiReader, res) return r.deleteResource(ctx, rm, res) } rlog := ackrtlog.FromContext(ctx) rlog.Info("AWS resource will not be deleted - deletion policy set to retain") - if err := r.setResourceUnmanaged(ctx, res); err != nil { + if err := r.setResourceUnmanaged(ctx, rm, res); err != nil { return res, err } return r.handleRequeues(ctx, res) @@ -246,22 +246,23 @@ func (r *resourceReconciler) Sync( rlog.WithValues("is_adopted", isAdopted) rlog.Enter("rm.ResolveReferences") - resolvedRefDesired, err := rm.ResolveReferences(ctx, r.apiReader, desired) + err = rm.ResolveReferences(ctx, r.apiReader, desired) rlog.Exit("rm.ResolveReferences", err) if err != nil { - return resolvedRefDesired, err + return desired, err } - desired = resolvedRefDesired + + resolved := rm.CopyWithResolvedReferences(desired) rlog.Enter("rm.EnsureTags") - err = rm.EnsureTags(ctx, desired, r.sc.GetMetadata()) + err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata()) rlog.Exit("rm.EnsureTags", err) if err != nil { - return desired, err + return resolved, err } rlog.Enter("rm.ReadOne") - latest, err = rm.ReadOne(ctx, desired) + latest, err = rm.ReadOne(ctx, resolved) rlog.Exit("rm.ReadOne", err) if err != nil { if err != ackerr.NotFound { @@ -270,11 +271,11 @@ func (r *resourceReconciler) Sync( if isAdopted { return nil, ackerr.AdoptedResourceNotFound } - if latest, err = r.createResource(ctx, rm, desired); err != nil { + if latest, err = r.createResource(ctx, rm, resolved); err != nil { return latest, err } } else { - if latest, err = r.updateResource(ctx, rm, desired, latest); err != nil { + if latest, err = r.updateResource(ctx, rm, resolved, latest); err != nil { return latest, err } } @@ -395,22 +396,10 @@ func (r *resourceReconciler) createResource( // manages the resource OR if the backend AWS service resource is // properly deleted. if !r.rd.IsManaged(desired) { - if err = r.setResourceManaged(ctx, desired); err != nil { + if err = r.setResourceManaged(ctx, rm, desired); err != nil { return nil, err } - // Resolve the references again after adding the finalizer and - // patching the resource. Patching resource omits the resolved references - // because they are not persisted in etcd. So we resolve the references - // again before performing the create operation. - rlog.Enter("rm.ResolveReferences") - resolvedRefDesired, err := rm.ResolveReferences(ctx, r.apiReader, desired) - rlog.Exit("rm.ResolveReferences", err) - if err != nil { - return resolvedRefDesired, err - } - desired = resolvedRefDesired - // Ensure tags again after adding the finalizer and patching the // resource. Patching desired resource omits the controller tags // because they are not persisted in etcd. So we again ensure @@ -457,11 +446,12 @@ func (r *resourceReconciler) createResource( // Ensure that we are patching any changes to the annotations/metadata and // the Spec that may have been set by the resource manager's successful // Create call above. - err = r.patchResourceMetadataAndSpec(ctx, desired, latest) + latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest) if err != nil { return latest, err } rlog.Info("created new resource") + return latest, nil } @@ -550,7 +540,7 @@ func (r *resourceReconciler) updateResource( // Ensure that we are patching any changes to the annotations/metadata and // the Spec that may have been set by the resource manager's successful // Update call above. - err = r.patchResourceMetadataAndSpec(ctx, desired, latest) + latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest) if err != nil { return latest, err } @@ -589,7 +579,8 @@ func (r *resourceReconciler) lateInitializeResource( // This patching does not hurt because if there is no diff then 'patchResourceMetadataAndSpec' // acts as a no-op. if ackcompare.IsNotNil(lateInitializedLatest) { - patchErr := r.patchResourceMetadataAndSpec(ctx, latest, lateInitializedLatest) + var patchErr error + lateInitializedLatest, patchErr = r.patchResourceMetadataAndSpec(ctx, rm, latest, lateInitializedLatest) // Throw the patching error if reconciler is unable to patch the resource with late initializations if patchErr != nil { err = patchErr @@ -629,20 +620,21 @@ func getPatchDocument( // patchResourceMetadataAndSpec patches the custom resource in the Kubernetes // API to match the supplied latest resource's metadata and spec. // -// NOTE(jaypipes): The latest parameter is *mutated* by this method: the -// resource's metadata.resourceVersion is incremented in the process of calling -// Patch. This is intentional, because without updating the resource's -// metadata.resourceVersion, the resource cannot be passed to Patch again later -// in the reconciliation loop if Patch is called with the Optimistic Locking -// option. +// NOTE(redbackthomson): This method returns an updated version of the latest +// parameter. This version has an updated metadata.resourceVersion, which is +// incremented in the process of calling Patch. This is intentional, because +// without updating the resource's metadata.resourceVersion, the resource cannot +// be passed to Patch again later in the reconciliation if Patch is called with +// the Optimistic Locking option. // // See https://github.com/kubernetes-sigs/controller-runtime/blob/165a8c869c4388b861c7c91cb1e5330f6e07ee16/pkg/client/patch.go#L81-L84 // for more information. func (r *resourceReconciler) patchResourceMetadataAndSpec( ctx context.Context, + rm acktypes.AWSResourceManager, desired acktypes.AWSResource, latest acktypes.AWSResource, -) error { +) (acktypes.AWSResource, error) { var err error rlog := ackrtlog.FromContext(ctx) exit := rlog.Trace("r.patchResourceMetadataAndSpec") @@ -650,20 +642,23 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec( exit(err) }() - equalMetadata, err := ackcompare.MetaV1ObjectEqual(desired.MetaObject(), latest.MetaObject()) + // Remove resolved references from the objects before patching + desiredCleaned := rm.ExtractResolvedReferences(desired) + latestCleaned := rm.ExtractResolvedReferences(latest) + + equalMetadata, err := ackcompare.MetaV1ObjectEqual(desiredCleaned.MetaObject(), latestCleaned.MetaObject()) if err != nil { - return err + return latest, err } - if equalMetadata && !r.rd.Delta(desired, latest).DifferentAt("Spec") { + if equalMetadata && !r.rd.Delta(desiredCleaned, latestCleaned).DifferentAt("Spec") { rlog.Debug("no difference found between metadata and spec for desired and latest object.") - return nil + return latest, nil } rlog.Enter("kc.Patch (metadata + spec)") - dobj := desired.DeepCopy().RuntimeObject() - lorig := latest.DeepCopy() - patch := client.MergeFrom(dobj) - err = r.kc.Patch(ctx, latest.RuntimeObject(), patch) + lorig := latestCleaned.DeepCopy() + patch := client.MergeFrom(desiredCleaned.RuntimeObject()) + err = r.kc.Patch(ctx, latestCleaned.RuntimeObject(), patch) if err == nil { if rlog.IsDebugEnabled() { js := getPatchDocument(patch, lorig.RuntimeObject()) @@ -673,9 +668,9 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec( // The call to Patch() above ends up setting the latest variable's Status // to the value of the desired variable's Status. We do not want this // behaviour; instead, we want to keep latest's original Status value. - latest.SetStatus(lorig) + latestCleaned.SetStatus(lorig) rlog.Exit("kc.Patch (metadata + spec)", err) - return err + return latestCleaned, err } // patchResourceStatus patches the custom resource in the Kubernetes API to @@ -740,7 +735,7 @@ func (r *resourceReconciler) deleteResource( if err != nil { if err == ackerr.NotFound { // If the aws resource is not found, remove finalizer - return current, r.setResourceUnmanaged(ctx, current) + return current, r.setResourceUnmanaged(ctx, rm, current) } return current, err } @@ -756,7 +751,7 @@ func (r *resourceReconciler) deleteResource( // any changes to Status fields that may have been made by the resource // manager if the returned `latest` resource is non-nil, so we don't // have to worry about saving status stuff here. - _ = r.patchResourceMetadataAndSpec(ctx, current, latest) + latest, _ = r.patchResourceMetadataAndSpec(ctx, rm, current, latest) } if err != nil { // NOTE: Delete() implementations that have asynchronously-completing @@ -768,9 +763,9 @@ func (r *resourceReconciler) deleteResource( // up, we remove the finalizer representing the CR is managed by ACK, // allowing the CR to be deleted by the Kubernetes API server if ackcompare.IsNotNil(latest) { - err = r.setResourceUnmanaged(ctx, latest) + err = r.setResourceUnmanaged(ctx, rm, latest) } else { - err = r.setResourceUnmanaged(ctx, current) + err = r.setResourceUnmanaged(ctx, rm, current) } if err == nil { rlog.Info("deleted resource") @@ -784,6 +779,7 @@ func (r *resourceReconciler) deleteResource( // be deleted until that finalizer is removed (in setResourceUnmanaged()) func (r *resourceReconciler) setResourceManaged( ctx context.Context, + rm acktypes.AWSResourceManager, res acktypes.AWSResource, ) error { if r.rd.IsManaged(res) { @@ -798,7 +794,7 @@ func (r *resourceReconciler) setResourceManaged( orig := res.DeepCopy().RuntimeObject() r.rd.MarkManaged(res) - err = r.patchResourceMetadataAndSpec(ctx, r.rd.ResourceFromRuntimeObject(orig), res) + res, err = r.patchResourceMetadataAndSpec(ctx, rm, r.rd.ResourceFromRuntimeObject(orig), res) if err != nil { return err } @@ -811,6 +807,7 @@ func (r *resourceReconciler) setResourceManaged( // allows the CR to be deleted by the Kubernetes API server. func (r *resourceReconciler) setResourceUnmanaged( ctx context.Context, + rm acktypes.AWSResourceManager, res acktypes.AWSResource, ) error { if !r.rd.IsManaged(res) { @@ -826,7 +823,7 @@ func (r *resourceReconciler) setResourceUnmanaged( orig := res.DeepCopy().RuntimeObject() r.rd.MarkUnmanaged(res) - err = r.patchResourceMetadataAndSpec(ctx, r.rd.ResourceFromRuntimeObject(orig), res) + res, err = r.patchResourceMetadataAndSpec(ctx, rm, r.rd.ResourceFromRuntimeObject(orig), res) if err != nil { return err } diff --git a/pkg/types/aws_resource_manager.go b/pkg/types/aws_resource_manager.go index 2e449e1..aa88b01 100644 --- a/pkg/types/aws_resource_manager.go +++ b/pkg/types/aws_resource_manager.go @@ -18,7 +18,6 @@ import ( "github.com/aws/aws-sdk-go/aws/session" "github.com/go-logr/logr" - "sigs.k8s.io/controller-runtime/pkg/client" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" @@ -33,6 +32,7 @@ import ( // Use an AWSResourceManagerFactory to create an AWSResourceManager for a // particular APIResource and AWS account. type AWSResourceManager interface { + ResolvedReferenceManager // ReadOne returns the currently-observed state of the supplied AWSResource // in the backend AWS service API. // @@ -75,14 +75,6 @@ type AWSResourceManager interface { // object passed in the parameter. // This method also adds/updates the ConditionTypeLateInitialized for the AWSResource. LateInitialize(context.Context, AWSResource) (AWSResource, error) - // ResolveReferences finds if there are any Reference field(s) present - // inside AWSResource passed in the parameter and attempts to resolve - // those reference field(s) into target field(s). - // It returns an AWSResource with resolved reference(s), and an error if the - // passed AWSResource's reference field(s) cannot be resolved. - // This method also adds/updates the ConditionTypeReferencesResolved for the - // AWSResource. - ResolveReferences(context.Context, client.Reader, AWSResource) (AWSResource, error) // IsSynced returns true if a resource is synced. IsSynced(context.Context, AWSResource) (bool, error) // EnsureTags ensures that tags are present inside the AWSResource. diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/resolved_reference_manager.go new file mode 100644 index 0000000..f224ac3 --- /dev/null +++ b/pkg/types/resolved_reference_manager.go @@ -0,0 +1,38 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package types + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ResolvedReferenceManager describes a thing that can set and retrieve the +// value of resolved references within a resource. +type ResolvedReferenceManager interface { + // ResolveReferences finds if there are any Reference field(s) present + // inside AWSResource passed in the parameter and attempts to resolve + // those reference field(s) into target field(s). + // It returns an AWSResource with resolved reference(s), and an error if the + // passed AWSResource's reference field(s) cannot be resolved. + // This method also adds/updates the ConditionTypeReferencesResolved for the + // AWSResource. + ResolveReferences(context.Context, client.Reader, AWSResource) error + CopyWithResolvedReferences(AWSResource) AWSResource + // ClearResolvedReferences removes any reference values that were made + // concrete in the spec. It returns a copy of the spec which contains the + // original *Ref values, but none of their respective values. + ClearResolvedReferences(AWSResource) AWSResource +} From 9e67e59daec011f5e8afb9daab8a5d0796e5fc3d Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Wed, 5 Apr 2023 19:51:27 +0000 Subject: [PATCH 2/7] Update types to match latest code-gen output --- mocks/pkg/types/aws_resource_manager.go | 49 ++++++++-- mocks/pkg/types/resolved_reference_manager.go | 93 +++++++++++++++++++ pkg/condition/condition.go | 4 +- pkg/errors/resource_reference.go | 10 ++ pkg/runtime/reconciler.go | 18 +++- pkg/types/resolved_reference_manager.go | 16 ++-- 6 files changed, 170 insertions(+), 20 deletions(-) create mode 100644 mocks/pkg/types/resolved_reference_manager.go diff --git a/mocks/pkg/types/aws_resource_manager.go b/mocks/pkg/types/aws_resource_manager.go index 550d929..e25e4e7 100644 --- a/mocks/pkg/types/aws_resource_manager.go +++ b/mocks/pkg/types/aws_resource_manager.go @@ -32,6 +32,45 @@ func (_m *AWSResourceManager) ARNFromName(_a0 string) string { return r0 } +// ClearResolvedReferences provides a mock function with given fields: _a0 +func (_m *AWSResourceManager) ClearResolvedReferences(_a0 types.AWSResource) (types.AWSResource, error) { + ret := _m.Called(_a0) + + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(types.AWSResource) error); ok { + r1 = rf(_a0) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// CopyWithResolvedReferences provides a mock function with given fields: _a0 +func (_m *AWSResourceManager) CopyWithResolvedReferences(_a0 types.AWSResource) types.AWSResource { + ret := _m.Called(_a0) + + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } + } + + return r0 +} + // Create provides a mock function with given fields: _a0, _a1 func (_m *AWSResourceManager) Create(_a0 context.Context, _a1 types.AWSResource) (types.AWSResource, error) { ret := _m.Called(_a0, _a1) @@ -160,16 +199,14 @@ func (_m *AWSResourceManager) ReadOne(_a0 context.Context, _a1 types.AWSResource } // ResolveReferences provides a mock function with given fields: _a0, _a1, _a2 -func (_m *AWSResourceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (types.AWSResource, error) { +func (_m *AWSResourceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (bool, error) { ret := _m.Called(_a0, _a1, _a2) - var r0 types.AWSResource - if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) types.AWSResource); ok { + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) bool); ok { r0 = rf(_a0, _a1, _a2) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(types.AWSResource) - } + r0 = ret.Get(0).(bool) } var r1 error diff --git a/mocks/pkg/types/resolved_reference_manager.go b/mocks/pkg/types/resolved_reference_manager.go new file mode 100644 index 0000000..fcc0669 --- /dev/null +++ b/mocks/pkg/types/resolved_reference_manager.go @@ -0,0 +1,93 @@ +// Code generated by mockery v2.19.0. DO NOT EDIT. + +package mocks + +import ( + context "context" + + client "sigs.k8s.io/controller-runtime/pkg/client" + + mock "github.com/stretchr/testify/mock" + + types "github.com/aws-controllers-k8s/runtime/pkg/types" +) + +// ResolvedReferenceManager is an autogenerated mock type for the ResolvedReferenceManager type +type ResolvedReferenceManager struct { + mock.Mock +} + +// ClearResolvedReferences provides a mock function with given fields: _a0 +func (_m *ResolvedReferenceManager) ClearResolvedReferences(_a0 types.AWSResource) (types.AWSResource, error) { + ret := _m.Called(_a0) + + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(types.AWSResource) error); ok { + r1 = rf(_a0) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// CopyWithResolvedReferences provides a mock function with given fields: _a0 +func (_m *ResolvedReferenceManager) CopyWithResolvedReferences(_a0 types.AWSResource) types.AWSResource { + ret := _m.Called(_a0) + + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } + } + + return r0 +} + +// ResolveReferences provides a mock function with given fields: _a0, _a1, _a2 +func (_m *ResolvedReferenceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (bool, error) { + ret := _m.Called(_a0, _a1, _a2) + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) bool); ok { + r0 = rf(_a0, _a1, _a2) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, client.Reader, types.AWSResource) error); ok { + r1 = rf(_a0, _a1, _a2) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +type mockConstructorTestingTNewResolvedReferenceManager interface { + mock.TestingT + Cleanup(func()) +} + +// NewResolvedReferenceManager creates a new instance of ResolvedReferenceManager. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewResolvedReferenceManager(t mockConstructorTestingTNewResolvedReferenceManager) *ResolvedReferenceManager { + mock := &ResolvedReferenceManager{} + 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 66526be..aed68a8 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -240,7 +240,7 @@ func RemoveReferencesResolved( func WithReferencesResolvedCondition( resource acktypes.AWSResource, err error, -) (acktypes.AWSResource, error) { +) acktypes.AWSResource { if err != nil { errString := err.Error() conditionStatus := corev1.ConditionUnknown @@ -251,7 +251,7 @@ func WithReferencesResolvedCondition( } else { SetReferencesResolved(resource, corev1.ConditionTrue, nil, nil) } - return resource, err + return resource } // LateInitializationInProgress return true if ConditionTypeLateInitialized has "False" status diff --git a/pkg/errors/resource_reference.go b/pkg/errors/resource_reference.go index 2cd46e6..1a68f0b 100644 --- a/pkg/errors/resource_reference.go +++ b/pkg/errors/resource_reference.go @@ -47,6 +47,10 @@ var ( ResourceReferenceMissingTargetField = fmt.Errorf( "the referenced resource is missing the target field", ) + // ResourceReferenceValueCastFailed indicates that the resolved reference + // could not be cast back to the type that was used to set it in the map + ResourceReferenceValueCastFailed = fmt.Errorf("the resolved reference could not be cast" + + " back to its original type") ) // ResourceReferenceOrIDRequiredFor returns a ResourceReferenceOrIDRequired error @@ -90,3 +94,9 @@ func ResourceReferenceMissingTargetFieldFor(resource string, namespace string, ", targetField:%s", ResourceReferenceMissingTargetField, resource, namespace, name, targetField) } + +// ResourceReferenceValueCastFailedFor returns a +// ResourceReferenceValueCastFailed for the supplied field +func ResourceReferenceValueCastFailedFor(field string) error { + return fmt.Errorf("%w: %s", ResourceReferenceValueCastFailed, field) +} diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index cf08c44..4c685de 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -201,7 +201,7 @@ func (r *resourceReconciler) reconcile( if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete { // Resolve references before deleting the resource. // Ignore any errors while resolving the references - _ = rm.ResolveReferences(ctx, r.apiReader, res) + _, _ = rm.ResolveReferences(ctx, r.apiReader, res) return r.deleteResource(ctx, rm, res) } @@ -246,8 +246,11 @@ func (r *resourceReconciler) Sync( rlog.WithValues("is_adopted", isAdopted) rlog.Enter("rm.ResolveReferences") - err = rm.ResolveReferences(ctx, r.apiReader, desired) + hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired) rlog.Exit("rm.ResolveReferences", err) + if hasReferences { + desired = ackcondition.WithReferencesResolvedCondition(desired, err) + } if err != nil { return desired, err } @@ -643,8 +646,15 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec( }() // Remove resolved references from the objects before patching - desiredCleaned := rm.ExtractResolvedReferences(desired) - latestCleaned := rm.ExtractResolvedReferences(latest) + desiredCleaned, err := rm.ClearResolvedReferences(desired) + if err != nil { + return latest, err + } + + latestCleaned, err := rm.ClearResolvedReferences(latest) + if err != nil { + return latest, err + } equalMetadata, err := ackcompare.MetaV1ObjectEqual(desiredCleaned.MetaObject(), latestCleaned.MetaObject()) if err != nil { diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/resolved_reference_manager.go index f224ac3..cbbbf86 100644 --- a/pkg/types/resolved_reference_manager.go +++ b/pkg/types/resolved_reference_manager.go @@ -24,15 +24,15 @@ import ( type ResolvedReferenceManager interface { // ResolveReferences finds if there are any Reference field(s) present // inside AWSResource passed in the parameter and attempts to resolve - // those reference field(s) into target field(s). - // It returns an AWSResource with resolved reference(s), and an error if the - // passed AWSResource's reference field(s) cannot be resolved. - // This method also adds/updates the ConditionTypeReferencesResolved for the - // AWSResource. - ResolveReferences(context.Context, client.Reader, AWSResource) error + // those reference field(s) into the resolved references map. + // It returns a boolean that is set to true if there the resource is using + // references, and an error if the passed AWSResource's reference field(s) + // cannot be resolved. + ResolveReferences(context.Context, client.Reader, AWSResource) (bool, error) CopyWithResolvedReferences(AWSResource) AWSResource // ClearResolvedReferences removes any reference values that were made // concrete in the spec. It returns a copy of the spec which contains the - // original *Ref values, but none of their respective values. - ClearResolvedReferences(AWSResource) AWSResource + // original *Ref values, but none of their respective values, and optionally + // an error. + ClearResolvedReferences(AWSResource) (AWSResource, error) } From 403d21d3501bd975125048f4fb830a5e6a10896b Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Wed, 5 Apr 2023 23:56:39 +0000 Subject: [PATCH 3/7] Update CopyWithResolvedReferences return types --- pkg/runtime/reconciler.go | 5 ++++- pkg/runtime/reconciler_test.go | 30 ++++++++++++------------- pkg/types/resolved_reference_manager.go | 2 +- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 4c685de..5082a48 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -255,7 +255,10 @@ func (r *resourceReconciler) Sync( return desired, err } - resolved := rm.CopyWithResolvedReferences(desired) + resolved, err := rm.CopyWithResolvedReferences(desired) + if err != nil { + return desired, err + } rlog.Enter("rm.EnsureTags") err = rm.EnsureTags(ctx, resolved, r.sc.GetMetadata()) diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 91427b1..67867c3 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -166,7 +166,7 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ).Times(2) rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, @@ -225,7 +225,7 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ).Times(2) rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, @@ -305,7 +305,7 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ).Once() rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, @@ -386,7 +386,7 @@ func TestReconcilerUpdate(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ).Once() rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -419,9 +419,9 @@ func TestReconcilerUpdate(t *testing.T) { // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) // 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, "ReadOne", ctx, desired) rd.AssertCalled(t, "Delta", desired, latest) rm.AssertCalled(t, "Update", ctx, desired, latest, delta) @@ -469,7 +469,7 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -550,7 +550,7 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -626,7 +626,7 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -706,7 +706,7 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -788,7 +788,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -865,7 +865,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -1006,7 +1006,7 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -1123,7 +1123,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - desired, nil, + false, nil, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -1188,7 +1188,7 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - nil, resolveReferenceError, + true, resolveReferenceError, ) rm.On("ReadOne", ctx, desired).Return( latest, nil, @@ -1272,7 +1272,7 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return(desired, nil) + rm.On("ResolveReferences", ctx, nil, desired).Return(false, nil) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/resolved_reference_manager.go index cbbbf86..e2659f6 100644 --- a/pkg/types/resolved_reference_manager.go +++ b/pkg/types/resolved_reference_manager.go @@ -29,7 +29,7 @@ type ResolvedReferenceManager interface { // references, and an error if the passed AWSResource's reference field(s) // cannot be resolved. ResolveReferences(context.Context, client.Reader, AWSResource) (bool, error) - CopyWithResolvedReferences(AWSResource) AWSResource + CopyWithResolvedReferences(AWSResource) (AWSResource, error) // ClearResolvedReferences removes any reference values that were made // concrete in the spec. It returns a copy of the spec which contains the // original *Ref values, but none of their respective values, and optionally From c27dcfdcd36c514589daba10d0a832cee3e309c9 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Thu, 6 Apr 2023 21:37:06 +0000 Subject: [PATCH 4/7] Revert back to inline reference resolution --- mocks/pkg/types/aws_resource_manager.go | 50 +++++------- mocks/pkg/types/resolved_reference_manager.go | 46 ++++------- pkg/condition/condition.go | 19 +++-- pkg/condition/condition_test.go | 9 ++- pkg/runtime/reconciler.go | 28 ++----- pkg/runtime/reconciler_test.go | 79 +++++++++++++------ pkg/types/resolved_reference_manager.go | 16 ++-- 7 files changed, 121 insertions(+), 126 deletions(-) diff --git a/mocks/pkg/types/aws_resource_manager.go b/mocks/pkg/types/aws_resource_manager.go index e25e4e7..3ba7ae7 100644 --- a/mocks/pkg/types/aws_resource_manager.go +++ b/mocks/pkg/types/aws_resource_manager.go @@ -33,30 +33,7 @@ func (_m *AWSResourceManager) ARNFromName(_a0 string) string { } // ClearResolvedReferences provides a mock function with given fields: _a0 -func (_m *AWSResourceManager) ClearResolvedReferences(_a0 types.AWSResource) (types.AWSResource, error) { - ret := _m.Called(_a0) - - var r0 types.AWSResource - if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { - r0 = rf(_a0) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(types.AWSResource) - } - } - - var r1 error - if rf, ok := ret.Get(1).(func(types.AWSResource) error); ok { - r1 = rf(_a0) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// CopyWithResolvedReferences provides a mock function with given fields: _a0 -func (_m *AWSResourceManager) CopyWithResolvedReferences(_a0 types.AWSResource) types.AWSResource { +func (_m *AWSResourceManager) ClearResolvedReferences(_a0 types.AWSResource) types.AWSResource { ret := _m.Called(_a0) var r0 types.AWSResource @@ -199,24 +176,33 @@ func (_m *AWSResourceManager) ReadOne(_a0 context.Context, _a1 types.AWSResource } // ResolveReferences provides a mock function with given fields: _a0, _a1, _a2 -func (_m *AWSResourceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (bool, error) { +func (_m *AWSResourceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (types.AWSResource, bool, error) { ret := _m.Called(_a0, _a1, _a2) - var r0 bool - if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) bool); ok { + var r0 types.AWSResource + if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) types.AWSResource); ok { r0 = rf(_a0, _a1, _a2) } else { - r0 = ret.Get(0).(bool) + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.AWSResource) + } } - var r1 error - if rf, ok := ret.Get(1).(func(context.Context, client.Reader, types.AWSResource) error); ok { + var r1 bool + if rf, ok := ret.Get(1).(func(context.Context, client.Reader, types.AWSResource) bool); ok { r1 = rf(_a0, _a1, _a2) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(bool) } - return r0, r1 + var r2 error + if rf, ok := ret.Get(2).(func(context.Context, client.Reader, types.AWSResource) error); ok { + r2 = rf(_a0, _a1, _a2) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } // Update provides a mock function with given fields: _a0, _a1, _a2, _a3 diff --git a/mocks/pkg/types/resolved_reference_manager.go b/mocks/pkg/types/resolved_reference_manager.go index fcc0669..a9f3ec9 100644 --- a/mocks/pkg/types/resolved_reference_manager.go +++ b/mocks/pkg/types/resolved_reference_manager.go @@ -18,7 +18,7 @@ type ResolvedReferenceManager struct { } // ClearResolvedReferences provides a mock function with given fields: _a0 -func (_m *ResolvedReferenceManager) ClearResolvedReferences(_a0 types.AWSResource) (types.AWSResource, error) { +func (_m *ResolvedReferenceManager) ClearResolvedReferences(_a0 types.AWSResource) types.AWSResource { ret := _m.Called(_a0) var r0 types.AWSResource @@ -30,51 +30,37 @@ func (_m *ResolvedReferenceManager) ClearResolvedReferences(_a0 types.AWSResourc } } - var r1 error - if rf, ok := ret.Get(1).(func(types.AWSResource) error); ok { - r1 = rf(_a0) - } else { - r1 = ret.Error(1) - } - - return r0, r1 + return r0 } -// CopyWithResolvedReferences provides a mock function with given fields: _a0 -func (_m *ResolvedReferenceManager) CopyWithResolvedReferences(_a0 types.AWSResource) types.AWSResource { - ret := _m.Called(_a0) +// ResolveReferences provides a mock function with given fields: _a0, _a1, _a2 +func (_m *ResolvedReferenceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (types.AWSResource, bool, error) { + ret := _m.Called(_a0, _a1, _a2) var r0 types.AWSResource - if rf, ok := ret.Get(0).(func(types.AWSResource) types.AWSResource); ok { - r0 = rf(_a0) + if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) types.AWSResource); ok { + r0 = rf(_a0, _a1, _a2) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(types.AWSResource) } } - return r0 -} - -// ResolveReferences provides a mock function with given fields: _a0, _a1, _a2 -func (_m *ResolvedReferenceManager) ResolveReferences(_a0 context.Context, _a1 client.Reader, _a2 types.AWSResource) (bool, error) { - ret := _m.Called(_a0, _a1, _a2) - - var r0 bool - if rf, ok := ret.Get(0).(func(context.Context, client.Reader, types.AWSResource) bool); ok { - r0 = rf(_a0, _a1, _a2) + var r1 bool + if rf, ok := ret.Get(1).(func(context.Context, client.Reader, types.AWSResource) bool); ok { + r1 = rf(_a0, _a1, _a2) } else { - r0 = ret.Get(0).(bool) + r1 = ret.Get(1).(bool) } - var r1 error - if rf, ok := ret.Get(1).(func(context.Context, client.Reader, types.AWSResource) error); ok { - r1 = rf(_a0, _a1, _a2) + var r2 error + if rf, ok := ret.Get(2).(func(context.Context, client.Reader, types.AWSResource) error); ok { + r2 = rf(_a0, _a1, _a2) } else { - r1 = ret.Error(1) + r2 = ret.Error(2) } - return r0, r1 + return r0, r1, r2 } type mockConstructorTestingTNewResolvedReferenceManager interface { diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index aed68a8..bdc17b8 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -29,9 +29,10 @@ 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 creating a services.k8s.aws/AdoptedResource" - UnknownSyncedMessage = "Unable to determine if desired resource state matches latest observed state" - NotSyncedMessage = "Resource not synced" - SyncedMessage = "Resource synced successfully" + UnknownSyncedMessage = "Unable to determine if desired resource state matches latest observed state" + NotSyncedMessage = "Resource not synced" + SyncedMessage = "Resource synced successfully" + FailedReferenceResolutionMessage = "Reference resolution failed" ) // Synced returns the Condition in the resource's Conditions collection that is @@ -235,23 +236,25 @@ func RemoveReferencesResolved( } } -// WithReferencesResolvedCondition sets the ConditionTypeReferencesResolved in -// AWSResource based on the err parameter and returns (AWSResource,error) +// WithReferencesResolvedCondition returns a new AWSResource with the +// ConditionTypeReferencesResolved set based on the err parameter func WithReferencesResolvedCondition( resource acktypes.AWSResource, err error, ) acktypes.AWSResource { + ko := resource.DeepCopy() + if err != nil { errString := err.Error() conditionStatus := corev1.ConditionUnknown if strings.Contains(errString, ackerr.ResourceReferenceTerminal.Error()) { conditionStatus = corev1.ConditionFalse } - SetReferencesResolved(resource, conditionStatus, &errString, nil) + SetReferencesResolved(ko, conditionStatus, &FailedReferenceResolutionMessage, &errString) } else { - SetReferencesResolved(resource, corev1.ConditionTrue, nil, nil) + SetReferencesResolved(ko, corev1.ConditionTrue, &FailedReferenceResolutionMessage, nil) } - return resource + return ko } // LateInitializationInProgress return true if ConditionTypeLateInitialized has "False" status diff --git a/pkg/condition/condition_test.go b/pkg/condition/condition_test.go index 6070d49..c27786f 100644 --- a/pkg/condition/condition_test.go +++ b/pkg/condition/condition_test.go @@ -34,6 +34,7 @@ func TestConditionGetters(t *testing.T) { conds := []*ackv1alpha1.Condition{} r := &ackmocks.AWSResource{} + r.On("DeepCopy").Return(r) r.On("Conditions").Return(conds) got := ackcond.Synced(r) @@ -245,6 +246,7 @@ func TestConditionSetters(t *testing.T) { //WithReferencesResolvedCondition // Without Error r = &ackmocks.AWSResource{} + r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( "ReplaceConditions", @@ -261,6 +263,7 @@ func TestConditionSetters(t *testing.T) { errorMsg := "error message" err := errors.New(errorMsg) r = &ackmocks.AWSResource{} + r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( "ReplaceConditions", @@ -270,13 +273,14 @@ func TestConditionSetters(t *testing.T) { } return (subject[0].Type == ackv1alpha1.ConditionTypeReferencesResolved && subject[0].Status == corev1.ConditionUnknown && - *subject[0].Message == errorMsg) + *subject[0].Message == ackcond.FailedReferenceResolutionMessage) }), ) ackcond.WithReferencesResolvedCondition(r, err) // With Terminal Error terminalError := ackerr.ResourceReferenceTerminal r = &ackmocks.AWSResource{} + r.On("DeepCopy").Return(r) r.On("Conditions").Return([]*ackv1alpha1.Condition{}) r.On( "ReplaceConditions", @@ -286,7 +290,8 @@ func TestConditionSetters(t *testing.T) { } return (subject[0].Type == ackv1alpha1.ConditionTypeReferencesResolved && subject[0].Status == corev1.ConditionFalse && - *subject[0].Message == terminalError.Error()) + *subject[0].Message == ackcond.FailedReferenceResolutionMessage && + *subject[0].Reason == terminalError.Error()) }), ) ackcond.WithReferencesResolvedCondition(r, terminalError) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 5082a48..23d9292 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -201,8 +201,8 @@ func (r *resourceReconciler) reconcile( if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete { // Resolve references before deleting the resource. // Ignore any errors while resolving the references - _, _ = rm.ResolveReferences(ctx, r.apiReader, res) - return r.deleteResource(ctx, rm, res) + resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, res) + return r.deleteResource(ctx, rm, resolved) } rlog := ackrtlog.FromContext(ctx) @@ -246,18 +246,13 @@ func (r *resourceReconciler) Sync( rlog.WithValues("is_adopted", isAdopted) rlog.Enter("rm.ResolveReferences") - hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired) + resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired) rlog.Exit("rm.ResolveReferences", err) - if hasReferences { - desired = ackcondition.WithReferencesResolvedCondition(desired, err) - } if err != nil { - return desired, err + return ackcondition.WithReferencesResolvedCondition(desired, err), err } - - resolved, err := rm.CopyWithResolvedReferences(desired) - if err != nil { - return desired, err + if hasReferences { + resolved = ackcondition.WithReferencesResolvedCondition(resolved, err) } rlog.Enter("rm.EnsureTags") @@ -649,15 +644,8 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec( }() // Remove resolved references from the objects before patching - desiredCleaned, err := rm.ClearResolvedReferences(desired) - if err != nil { - return latest, err - } - - latestCleaned, err := rm.ClearResolvedReferences(latest) - if err != nil { - return latest, err - } + desiredCleaned := rm.ClearResolvedReferences(desired) + latestCleaned := rm.ClearResolvedReferences(latest) equalMetadata, err := ackcompare.MetaV1ObjectEqual(desiredCleaned.MetaObject(), latestCleaned.MetaObject()) if err != nil { diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 67867c3..2a1db22 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -166,8 +166,10 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ).Times(2) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, ).Once() @@ -196,7 +198,7 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { rm.AssertNumberOfCalls(t, "ReadOne", 6) } -func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testing.T) { +func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveOnce(t *testing.T) { require := require.New(t) ctx := context.TODO() @@ -225,8 +227,10 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ).Times(2) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, ).Once() @@ -260,9 +264,10 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi // method, _, err := r.Sync(ctx, rm, desired) require.Nil(err) - // Make sure references are resolved twice for the resource creation. - // Once before ReadOne call and one after marking the resource managed. - rm.AssertNumberOfCalls(t, "ResolveReferences", 2) + // Make sure references are only resolved once for the resource creation. + // 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, "ReadOne", ctx, desired) rm.AssertCalled(t, "Create", ctx, desired) @@ -305,8 +310,10 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ).Once() + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, ackerr.NotFound, ).Once() @@ -386,8 +393,10 @@ func TestReconcilerUpdate(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ).Once() + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -469,8 +478,10 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -550,8 +561,10 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -626,8 +639,10 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -706,8 +721,10 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -788,8 +805,10 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -865,8 +884,10 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -1006,8 +1027,10 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -1123,8 +1146,10 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - false, nil, + desired, false, nil, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -1170,26 +1195,28 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { // manager has not set any Conditions on the resource, that at least an // ACK.ResourceSynced condition with status Unknown will be set on the // resource. - latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) - latest.On( + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On( "ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition"), ).Return().Run(func(args mock.Arguments) { conditions := args.Get(0).([]*ackv1alpha1.Condition) assert.Equal(t, 1, len(conditions)) cond := conditions[0] - assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type) - // The non-terminal reconciler error causes the ResourceSynced - // condition to be False - assert.Equal(t, corev1.ConditionFalse, cond.Status) - assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message) + assert.Equal(t, ackv1alpha1.ConditionTypeReferencesResolved, cond.Type) + // The non-terminal reconciler error causes the ReferencesResolved + // condition to be Unknown + assert.Equal(t, corev1.ConditionUnknown, cond.Status) + assert.Equal(t, ackcondition.FailedReferenceResolutionMessage, *cond.Message) assert.Equal(t, resolveReferenceError.Error(), *cond.Reason) }) rm := &ackmocks.AWSResourceManager{} rm.On("ResolveReferences", ctx, nil, desired).Return( - true, resolveReferenceError, + desired, true, resolveReferenceError, ) + rm.On("ClearResolvedReferences", desired).Return(desired) + rm.On("ClearResolvedReferences", latest).Return(latest) rm.On("ReadOne", ctx, desired).Return( latest, nil, ) @@ -1272,7 +1299,9 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { }) rm := &ackmocks.AWSResourceManager{} - rm.On("ResolveReferences", ctx, nil, desired).Return(false, nil) + 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( latest, nil, ) diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/resolved_reference_manager.go index e2659f6..9a92fa0 100644 --- a/pkg/types/resolved_reference_manager.go +++ b/pkg/types/resolved_reference_manager.go @@ -24,15 +24,13 @@ import ( type ResolvedReferenceManager interface { // ResolveReferences finds if there are any Reference field(s) present // inside AWSResource passed in the parameter and attempts to resolve - // those reference field(s) into the resolved references map. - // It returns a boolean that is set to true if there the resource is using - // references, and an error if the passed AWSResource's reference field(s) - // cannot be resolved. - ResolveReferences(context.Context, client.Reader, AWSResource) (bool, error) - CopyWithResolvedReferences(AWSResource) (AWSResource, error) + // those reference field(s) into target field(s). + // It returns an AWSResource with resolved reference(s), a boolean which is + // set to true if the resource contains references and an error if the + // passed AWSResource's reference field(s) cannot be resolved. + ResolveReferences(context.Context, client.Reader, AWSResource) (AWSResource, bool, error) // ClearResolvedReferences removes any reference values that were made // concrete in the spec. It returns a copy of the spec which contains the - // original *Ref values, but none of their respective values, and optionally - // an error. - ClearResolvedReferences(AWSResource) (AWSResource, error) + // original *Ref values, but none of their respective values. + ClearResolvedReferences(AWSResource) AWSResource } From a8646a032edd748e73dcef71db4ff561823c9953 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Thu, 6 Apr 2023 23:15:22 +0000 Subject: [PATCH 5/7] Update GoDocs --- pkg/types/resolved_reference_manager.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/resolved_reference_manager.go index 9a92fa0..8201b33 100644 --- a/pkg/types/resolved_reference_manager.go +++ b/pkg/types/resolved_reference_manager.go @@ -23,14 +23,16 @@ import ( // value of resolved references within a resource. type ResolvedReferenceManager interface { // ResolveReferences finds if there are any Reference field(s) present - // inside AWSResource passed in the parameter and attempts to resolve - // those reference field(s) into target field(s). - // It returns an AWSResource with resolved reference(s), a boolean which is - // set to true if the resource contains references and an error if the - // passed AWSResource's reference field(s) cannot be resolved. + // inside AWSResource passed in the parameter and attempts to resolve those + // reference field(s) into their respective target field(s). It returns a + // copy of the input AWSResource with resolved reference(s), a boolean which + // is set to true if the resource contains any references (regardless of if + // they are resolved successfully) and an error if the passed AWSResource's + // reference field(s) could not be resolved. ResolveReferences(context.Context, client.Reader, AWSResource) (AWSResource, bool, error) // ClearResolvedReferences removes any reference values that were made - // concrete in the spec. It returns a copy of the spec which contains the - // original *Ref values, but none of their respective values. + // concrete in the spec. It returns a copy of the input AWSResource which + // contains the original *Ref values, but none of their respective concrete + // values. ClearResolvedReferences(AWSResource) AWSResource } From 8c20c930e8606683d26b2eb9ef3c7bcfab515555 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Thu, 6 Apr 2023 23:17:49 +0000 Subject: [PATCH 6/7] Rename ReferenceManager --- pkg/errors/resource_reference.go | 10 ---------- pkg/types/aws_resource_manager.go | 2 +- ...olved_reference_manager.go => reference_manager.go} | 6 +++--- 3 files changed, 4 insertions(+), 14 deletions(-) rename pkg/types/{resolved_reference_manager.go => reference_manager.go} (90%) diff --git a/pkg/errors/resource_reference.go b/pkg/errors/resource_reference.go index 1a68f0b..2cd46e6 100644 --- a/pkg/errors/resource_reference.go +++ b/pkg/errors/resource_reference.go @@ -47,10 +47,6 @@ var ( ResourceReferenceMissingTargetField = fmt.Errorf( "the referenced resource is missing the target field", ) - // ResourceReferenceValueCastFailed indicates that the resolved reference - // could not be cast back to the type that was used to set it in the map - ResourceReferenceValueCastFailed = fmt.Errorf("the resolved reference could not be cast" + - " back to its original type") ) // ResourceReferenceOrIDRequiredFor returns a ResourceReferenceOrIDRequired error @@ -94,9 +90,3 @@ func ResourceReferenceMissingTargetFieldFor(resource string, namespace string, ", targetField:%s", ResourceReferenceMissingTargetField, resource, namespace, name, targetField) } - -// ResourceReferenceValueCastFailedFor returns a -// ResourceReferenceValueCastFailed for the supplied field -func ResourceReferenceValueCastFailedFor(field string) error { - return fmt.Errorf("%w: %s", ResourceReferenceValueCastFailed, field) -} diff --git a/pkg/types/aws_resource_manager.go b/pkg/types/aws_resource_manager.go index aa88b01..66001f8 100644 --- a/pkg/types/aws_resource_manager.go +++ b/pkg/types/aws_resource_manager.go @@ -32,7 +32,7 @@ import ( // Use an AWSResourceManagerFactory to create an AWSResourceManager for a // particular APIResource and AWS account. type AWSResourceManager interface { - ResolvedReferenceManager + ReferenceManager // ReadOne returns the currently-observed state of the supplied AWSResource // in the backend AWS service API. // diff --git a/pkg/types/resolved_reference_manager.go b/pkg/types/reference_manager.go similarity index 90% rename from pkg/types/resolved_reference_manager.go rename to pkg/types/reference_manager.go index 8201b33..b89044d 100644 --- a/pkg/types/resolved_reference_manager.go +++ b/pkg/types/reference_manager.go @@ -19,9 +19,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// ResolvedReferenceManager describes a thing that can set and retrieve the -// value of resolved references within a resource. -type ResolvedReferenceManager interface { +// ReferenceManager describes a thing that can resolve and clear references +// within an AWSResource. +type ReferenceManager interface { // ResolveReferences finds if there are any Reference field(s) present // inside AWSResource passed in the parameter and attempts to resolve those // reference field(s) into their respective target field(s). It returns a From eecad9370b14b260f380abee04c086d8237b6b92 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Fri, 7 Apr 2023 16:43:08 +0000 Subject: [PATCH 7/7] Remove fail message on res ref condition --- pkg/condition/condition.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index bdc17b8..226f553 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -252,7 +252,7 @@ func WithReferencesResolvedCondition( } SetReferencesResolved(ko, conditionStatus, &FailedReferenceResolutionMessage, &errString) } else { - SetReferencesResolved(ko, corev1.ConditionTrue, &FailedReferenceResolutionMessage, nil) + SetReferencesResolved(ko, corev1.ConditionTrue, nil, nil) } return ko }