Implementating ReferenceResolver in managed reconciler

[soorena776]

Signed-off-by: soorena776 javad@upbound.io

Description of your changes

Fixes crossplane/crossplane#887

Checklist

I have:

• Run make reviewable to ensure this PR is ready for review.
• Ensured this PR contains a neat, self documenting set of commits.
• Updated any relevant documentation, examples, or release notes.
• Updated the RBAC permissions in clusterrole.yaml to include any new types.

[negz]

Dealing in interface{} worries me a little, but I'm intrigued by this reflection based approach in general. A few comments on the implementation thus far.

[negz]

Are you sure this works?

If I follow correctly getter is effectively always r.reader.Get. That method needs to know the type metadata (i.e. the group, version and kind) of the resource it is asked to get - you can't look an object up in the Kubernetes API by namespace and name alone. Get determines the type metadata by using reflection to determine the name of the type contained within the runtime.Object interface and looking that type up in its scheme (which is pretty much a big map of type names to pointers to instances of those types). Therefore I wouldn't expect passing a nil interface to Get to work.

[soorena776]

You are right, it seems that the GKV is needed to retrieve an object. Do you think the following solution would make sense or would it be too noisy?

Add another method called RefereneKind to the AttributeReference, to make it look like:

type AttributeReferencer interface {

        // ReferenceKind returns an object that represents an instance of the referenced object
        ReferenceKind() runtime.Object

	// Build retrieves and builds the referenced attribute
	Build(getter LocalObjectGetter) (value string, err error)

	// Assign accepts a resource object, and assigns the given value to the corresponding property
	Assign(mg Managed, value string) error
}

and then populating ReferenceKind like

func (s *Subnet)ReferneceKind() runtime.Object{
    return &VPC{}
}

[soorena776]

It seems weird that this level of details is needed to retrieve an object. According to the docs name and namespace should suffice to uniquely identify an object.

Namespaces provide a scope for names. Names of resources need to be unique within a namespace, but not across namespaces.

Maybe there is another api to retrieve an object given the name and namespace?

[soorena776]

It seems that a general Unstructured object also could be used. Digging more to see this still would need GKV.

[soorena776]

Another option would be to add a key to the resource:"attributereferencer kind=vpc.network.aws.crossplane.io/v1alpha2" tag

[negz]

Let me see if I understand correctly. For the GCP subnetwork:

• ValidateReady would use the supplied client.Client to validate that the GCP network was in state Ready, returning an error if it was not. It would not validate the Provider (which doesn't have a concept of readiness).

• Build would use the supplied LocalObjectGetter to get the Network and its Provider, then return a string formed from the two, e.g. /projects/id/global/networks/name.

• Assign would set the network field of the supplied Managed to the value returned by Build.

Am I interpreting this correctly? If so it seems like a good direction. My only comments would be:

• Do we need the kind struct tag in this design? It's not immediately obvious to me why.
• There's a lot of similarity between LocalObjectGetter and client.Client (or client.Reader). Could ValidateReady and Build take the same client-ish argument?

[soorena776]

Yes, this is exactly what I meant!

Do we need the kind struct tag in this design? It's not immediately obvious to me why.

No we don't. Sorry for the confusion, the comment that followed was meant for the case we are going the path of using tags.

There's a lot of similarity between LocalObjectGetter and client.Client (or client.Reader). Could ValidateReady and Build take the same client-ish argument?

Yes, I was exactly thinking the same, both signatures need to be equal.

The rational behind passing LocalObjectGetter is two things:

• validate only local references are being made (by only accepting such type as input)
• hide details like namespace and context.Context

However, looks like in the new design we do need to read objects possibly from other namespaces (provider in Subnetwork case). To solve this problem, I am going to pass a client.Reader and a context.Context, but in reflection step require that the only field in the implementer type is an embedded LocalObjectReference.

[negz]

This sounds good to me! Thanks for bearing with me on this design.

[muvaf]

• ValidateReady would use the supplied client.Client to validate that the GCP network was in state Ready, returning an error if it was not. It would not validate the Provider (which doesn't have a concept of readiness).
• Build would use the supplied LocalObjectGetter to get the Network and its Provider, then return a string formed from the two, e.g. /projects/id/global/networks/name.
• Assign would set the network field of the supplied Managed to the value returned by Build.

If I have 5 reference fields, I'd have to implement 15 different methods, all very similar to each other. My two cents, I feel like one Resolve method to do all this for one field is simpler, which would result in 5 methods.

Maybe, for simple fields that gets a value from the referred resource and assigns it to another field in the referrer, we might even be able to have a default implementation like SimpleFieldResolver that developer can just opt-in to use for specific fields. Not 100% sure if that's technically feasible though.

[soorena776]

@muvaf The subject of why breaking down the Resolve method to smaller ones, came up earlier in this thread #46 (comment)

breaking it down to smaller methods so the user is enforced to roughly follow the pattern of {ValidateReady,Build,Assign}

I understand that it implementing the AttributeReferencer interface would be noisy, but I think this scaffolding is easier to enforce at the time of development, and more useful in the managed resource (things like determining the condition state of the resolution).

As for creating SimpleFieldResolver, I agree. One example would be VPCIdResolver in AWS types that would be roughly the same (except the assign implementation), that I am considering to use, so all types that need to refer to a vpcId use that type and avoid defining their own.

[muvaf]

I am not sure of the value in us providing a default resolver, especially given the high complexity of (r *APIManagedReferenceResolver) ResolveReferences(ctx context.Context, mg Managed) that is necessary to capture many scenarios. First use of struct tag, panic possiblity, use of heavy reflection, iterating over fields of a struct; things we seem to be forced to do.

On the other hand, the difference in my responsibility as the stack implementor doesn't change as much when using the default resolver. When I read the one-pager, I imagined I needed to implement a function for a reference field similar to https://github.com/crossplaneio/crossplane-runtime/pull/46/files#r334722588 or an implementation with Build & Assign methods. Using the new ManagedReferenceResolver interface, implementor could provide a ResolverChain []ManagedReferenceResolver and we can call that chain just like Initializer and Finalizer chains. If any of them returns error, we don't proceed.

But I see that we are taking on the responsibility of discovering the fields that satisfies the needed interface by going through each field via reflection and call their resolve functions. Also, in the resulting picture we lose some type information that could actually benefit implementation. However, as stack implementor, there is not much difference for me. The dependency logic will be implemented by me in both cases but with the current structure I just don't have to put my resolvers in SetupWithManager but expect crossplane-runtime to discover them. I think I can't see how this trade-off makes sense.

[muvaf]

NotReadyError is implemented by 82 structs according to Goland IDE. I think it's not that specific of an interface for us to make its implementation a differentiation factor.

[soorena776]

Good to know. Are you suggesting to use a different name?

[muvaf]

It's actually because of too much similarity with error type. If NotReadyError interface has a method other than Error() string, that'd be sufficiently different I think. It'd still satisfy Error interface and others wouldn't satisfy NotReadyError since they don't know about that other method; maybe a method to get list of unresolved dependencies? #46 (comment)

[soorena776]

@muvaf I have changed the NotReadyError in the later commits, let me know what you think

[muvaf]

If dependency graph is not yet resolved, should we consider this as error? I mean it's an expected path of execution, not something that shows a fault in either input or output. IMO, having ResolveReferences somehow returning an array of unresolved references could make more sense. Error would be nil but if the array is not empty, we'd say resolve blocked and possibly list the kind/name of the resources that are blocking.

[muvaf]

• ValidateReady would use the supplied client.Client to validate that the GCP network was in state Ready, returning an error if it was not. It would not validate the Provider (which doesn't have a concept of readiness).
• Build would use the supplied LocalObjectGetter to get the Network and its Provider, then return a string formed from the two, e.g. /projects/id/global/networks/name.
• Assign would set the network field of the supplied Managed to the value returned by Build.

If I have 5 reference fields, I'd have to implement 15 different methods, all very similar to each other. My two cents, I feel like one Resolve method to do all this for one field is simpler, which would result in 5 methods.

Maybe, for simple fields that gets a value from the referred resource and assigns it to another field in the referrer, we might even be able to have a default implementation like SimpleFieldResolver that developer can just opt-in to use for specific fields. Not 100% sure if that's technically feasible though.

[soorena776]

@muvaf thanks for the review!

I think my main goal here was to make the cross referencing as managed as possible, by laying out the overall required steps, and detecting the references automatically, rather than leaving it to the developer to write the logic and then just simply execute it by putting it in SetupWithManager . Taking this slightly more managed approach, has the following advantages:

• More readability, and visibility through using struct field tags
• Enforcing how resolution steps should look like
• Early panic if a referencer is not implemented correctly

That said, I agree that there is a trade-off here. It would be possible to go the Initializer route and avoid the reflection logic, but then the mentioned goodies won't be there. Given that I am already invested in this implementation, I prefer keeping the existing approach.

[negz]

Per discussion this morning I'd prefer us to remove this. It may be a good direction, but it would apply to all the places we execute implementations of interfaces that are supplied in stacks so I think we should defer for now and add this in general later if we think it's useful.

[negz]

This LGTM, pending the topics we discussed this morning. Specifically:

• Removing class level reference resolution if we can get away with it
• Removing the panic recovery logic
• Removing the superfluous IsConditionReady method.

All other comments (except the two missing tests) I've left should be considered non-blocking. I'd be happy to merge this PR without resolving them.