Update reference resolution interface types #121

RedbackThomson · 2023-04-06T23:21:16Z

Description of changes:
Creates a new ReferenceManager type that re-defines ResolveReferences and adds a new ClearResolvedReferences methods to ResourceManager. ResolveReferences now returns a copy of the original AWSResource. ClearResolvedReferences also returns a copy of the original AWSResource.

The reconciler now resolves the references once at the beginning and passes around the resolved versions of the resource. Right before the resource is patched, in patchMetadataAndSpec, the resource is cleaned of references so that they are not persisted to etcd.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly

I understand the need for those changes for now and overall it makes sense!
Kinda a low level review from my end. I mostly have questions but most importantly i want to bring this:
One think that is crossing my mind now: Should we NOT allow users to have a mix between referenced and non-referenced data for the same field?

pkg/condition/condition.go

a-hilaly · 2023-04-07T12:04:38Z

pkg/types/reference_manager.go

+	// 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
+	// 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)


Why is it important to know whether a resource has references or not? Couldn't we just rely on err == nil || err != nil ?

There are 3 cases:

References exist, and resolution was successful

References exist, but resolution was not successful

References don't exist

err only covers cases 1 and 2. Case 3 doesn't execute anything, so it'll never return an err. The only way we know if references exist, then, is to return a bool to say we at least attempted to resolve them.

a-hilaly · 2023-04-07T12:13:33Z

pkg/runtime/reconciler.go

@@ -811,6 +808,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,


Shall we just make rm part of resourceReconciler instead of passing it around between functions?

a-hilaly · 2023-04-07T12:41:23Z

pkg/types/reference_manager.go

+	// 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)


Same here maybe make client.Reader part of the implementation?

jaypipes

Still thinking this through but I think these changes make sense. :+1

jaypipes · 2023-04-08T17:17:33Z

pkg/runtime/reconciler.go

+// 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.


Perhaps update this note to say that the returned resource has its resource references removed?

jljaco · 2023-04-11T23:48:55Z

pkg/runtime/reconciler.go

-	patch := client.MergeFrom(dobj)
-	err = r.kc.Patch(ctx, latest.RuntimeObject(), patch)
+	lorig := latestCleaned.DeepCopy()
+	patch := client.MergeFrom(desiredCleaned.RuntimeObject())


noting that this is now effectively

client.MergeFrom(rm.ClearResolvedReferences(desired).RuntimeObject())

vs previously

client.MergeFrom(desired.DeepCopy().RuntimeObject())

I'm not 100% positive from reading the corresponding code-generator change -- but I think that ClearResolvedReferences does not return a deep copy of the resource, so you're effectively doing away with the deep copy part here. Does that matter? Is it intentional?

I made sure that all of the reference resolution-related methods returned copies. You can see this in the ClearResolvedReferences method - https://gist.github.com/RedbackThomson/8e11cbbe96065a4eb812c387665c747d#file-references-go-L34

a-hilaly

👍

jljaco · 2023-04-13T15:55:14Z

/lgtm

ack-prow · 2023-04-13T15:55:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, jljaco, RedbackThomson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,RedbackThomson,jaypipes,jljaco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

#435) Implements aws-controllers-k8s/runtime#121 Description of changes: This PR abstracts out the process of iterating through all references into a separate method called `iterReferenceValues`. `iterReferenceValues` produces Go code that drills down into the spec for every ref, iterating through any slices as necessary. Once it reaches the ref object, it calls a callback which can be used by other methods for logic pertaining to accessing the refs. This change allows `ResolveReferencesForField` to access all refs within lists of structs (or even within lists of lists of structs, etc.). `iterReferenceValues` is also used to implement `ClearResolvedReferencesForField`, which simply sets the concrete value to `nil` if it detects a non-nil value in the ref field (or `len > 0` for lists of refs). This PR also removes `hasNonNilReferences`. `hasNonNilReferences` was being used to indicate whether there were any references inside the resource, which required another set of iterating through all ref fields. Instead, each `resolveReferencesFor*` returns a boolean which indicates whether it found a reference during its iteration. Below is a Gist that shows the output of running the code-generator on the current EC2 `generator.yaml`: https://gist.github.com/RedbackThomson/8e11cbbe96065a4eb812c387665c747d For testing: `ec2` and `lambda` already have a series of tests for references. Both of these tests are passing, but they were also passing before this PR. I have a branch of the `ec2-controller` with additional tests using `RouteTables`, which have lists of structs containing refs, and all of those are passing as well. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Nicholas Thomson added 6 commits April 5, 2023 21:15

Add interfaces for separate reference resolution

4684042

Update types to match latest code-gen output

9e67e59

Update CopyWithResolvedReferences return types

403d21d

Revert back to inline reference resolution

c27dcfd

Update GoDocs

a8646a0

Rename ReferenceManager

8c20c93

ack-prow bot requested review from jljaco and vijtrip2 April 6, 2023 23:21

ack-prow bot added the approved label Apr 6, 2023

RedbackThomson mentioned this pull request Apr 6, 2023

Support lists of structs containing refs and ClearResolvedReferences aws-controllers-k8s/code-generator#435

Merged

jaypipes requested review from jaypipes and a-hilaly and removed request for vijtrip2 April 7, 2023 14:15

a-hilaly reviewed Apr 7, 2023

View reviewed changes

Remove fail message on res ref condition

eecad93

jaypipes approved these changes Apr 8, 2023

View reviewed changes

jljaco linked an issue Apr 11, 2023 that may be closed by this pull request

List of structs containing references are patched without references aws-controllers-k8s/community#1291

Closed

jljaco reviewed Apr 11, 2023

View reviewed changes

a-hilaly approved these changes Apr 12, 2023

View reviewed changes

ack-prow bot assigned jljaco Apr 13, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2023

ack-prow bot merged commit af66d1b into aws-controllers-k8s:main Apr 13, 2023

jljaco removed their assignment Apr 13, 2023

RedbackThomson deleted the upgrade-resolve-references branch April 13, 2023 16:02

smcavallo mentioned this pull request Apr 27, 2023

Support lists of structs containing refs and ClearResolvedReferences aws-controllers-k8s/eks-controller#67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update reference resolution interface types #121

Update reference resolution interface types #121

RedbackThomson commented Apr 6, 2023

a-hilaly left a comment

a-hilaly Apr 7, 2023

RedbackThomson Apr 7, 2023

a-hilaly Apr 7, 2023

a-hilaly Apr 7, 2023

jaypipes left a comment

jaypipes Apr 8, 2023

jljaco Apr 11, 2023

RedbackThomson Apr 12, 2023

a-hilaly left a comment

jljaco commented Apr 13, 2023

ack-prow bot commented Apr 13, 2023

Update reference resolution interface types #121

Update reference resolution interface types #121

Conversation

RedbackThomson commented Apr 6, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly Apr 7, 2023

Choose a reason for hiding this comment

RedbackThomson Apr 7, 2023

Choose a reason for hiding this comment

a-hilaly Apr 7, 2023

Choose a reason for hiding this comment

a-hilaly Apr 7, 2023

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

jaypipes Apr 8, 2023

Choose a reason for hiding this comment

jljaco Apr 11, 2023

Choose a reason for hiding this comment

RedbackThomson Apr 12, 2023

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

jljaco commented Apr 13, 2023

ack-prow bot commented Apr 13, 2023