Add a transformer to remove API group imports for cross-resource references

[ulucinar]

Description of your changes

Fixes #96

While working in the context of the multi-version CRD & conversion function generation pipelines and #321 & crossplane-contrib/provider-upjet-aws#1075, we observed that selectively moving individual resources to different API versions increases the likelihood of #96 because cross-resource references are more common inside an API group. We observed multiple cases of #96 in crossplane-contrib/provider-upjet-aws#1075 when we bumped the API versions of MRs with breaking API changes, such as the RoutingProfile.connect resource.

This PR employs a dynamic type registry in the calls to the APIResolver so that we don't need to import the modules for the individual GVKs in the resolver modules, which effectively eliminates the issue reported in #96.

While the approach taken here can be applied to the other providers in the ecosystem by implementing the logic in https://github.com/crossplane/crossplane-tools, we have decided to initially implement it in upjet as a transformer (as opposed to being a full-fledged generator). The crossplane-tools' angryjet is invoked first to generate the cross-resource reference resolvers (with cross API-group import statements) and then the transformer introduced with this change (resolver) can be applied as part of the build pipeline to get rid of the cross API-group import statements, effectively addressing #96 for upjet-based providers.

An example transformation obtained by running resolver is as follows:

// ResolveReferences of this Subnet.
func (mg *Subnet) ResolveReferences(ctx context.Context, c client.Reader) error {
	r := reference.NewAPIResolver(c, mg)

	var rsp reference.ResolutionResponse
	var err error

	rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
		CurrentValue: reference.FromPtrValue(mg.Spec.ForProvider.VPCID),
		Extract:      reference.ExternalName(),
		Reference:    mg.Spec.ForProvider.VPCIDRef,
		Selector:     mg.Spec.ForProvider.VPCIDSelector,
		To: reference.To{
			List:    &VPCList{},
			Managed: &VPC{},
		},
	})
	if err != nil {
		return errors.Wrap(err, "mg.Spec.ForProvider.VPCID")
	}
	mg.Spec.ForProvider.VPCID = reference.ToPtrValue(rsp.ResolvedValue)
	mg.Spec.ForProvider.VPCIDRef = rsp.ResolvedReference

is transformed into:

// ResolveReferences of this Subnet.
func (mg *Subnet) ResolveReferences(ctx context.Context, c client.Reader) error {
	var m xpresource.Managed
	var l xpresource.ManagedList
	r := reference.NewAPIResolver(c, mg)

	var rsp reference.ResolutionResponse
	var err error
	{
		m, l, err = apisresolver.GetManagedResource("ec2.aws.upbound.io",
			"v1beta1",
			"VPC", "VPCList",
		)
		if err != nil {
			return errors.Wrap(err, "failed to get the reference target managed resource and its list for reference resolution")
		}

		rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
			CurrentValue: reference.FromPtrValue(mg.Spec.ForProvider.VPCID),
			Extract:      reference.ExternalName(),
			Reference:    mg.Spec.ForProvider.VPCIDRef,
			Selector:     mg.Spec.ForProvider.VPCIDSelector,
			To:           reference.To{List: l, Managed: m},
		})
	}
	if err != nil {
		return errors.Wrap(err, "mg.Spec.ForProvider.VPCID")
	}
	mg.Spec.ForProvider.VPCID = reference.ToPtrValue(rsp.ResolvedValue)
	mg.Spec.ForProvider.VPCIDRef = rsp.ResolvedReference

TODO:

• Implement unit tests

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
• Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

All the generated resolver modules for upbound/provider-aws have been transformed using resolver in crossplane-contrib/provider-upjet-aws#1075. We have also tried moving the RoutingProfile.connect resource to version v1beta2, leaving all the other resources in the same API group at version v1beta1, made sure the import cycle exists between connect/v1beta1/zz_generated.resolvers.go & connect/v1beta2/zz_generated.resolvers.go, and applied to the transformer to remove the import cycle, and then successfully provision a RoutingProfile.connect instance using this manifest.

[sergenyalcin]

Thanks @ulucinar I left a few comments

[sergenyalcin]

What about making the default of this option to true?

[ulucinar]

I feel like the default should be the more strict mode of operation (failing on package loading errors). Though I think I see your point: The purpose of this transformer is to prevent/get rid of circular imports, and any provider benefiting from this transformer should have an import cycle caused by the cross-API group references, so in order to run this transformer, they will need to supply the -s option. But still, I'd like the decision to ignore any package loading errors to be on the side of the Crossplane provider repo maintainer who's using this transformer as part of their code generation pipelines. So keeping the default mode strict forces the repo maintainer to show explicit consent while incorporating the transformer.

The package loading issues we expect are the import cycle errors caused by cross-resource references but what if the API folder has some unexpected (by the transformer) compile errors? I believe it's better to give the responsibility of suppressing package loading errors to the repo maintainer, although from a practical perspective, I agree setting the default of this option to true will not make much difference, if this is the reasoning behind the proposal.

[sergenyalcin]

In my local tests, I observe that the place of the first resolver function's code comment changes. This is not a very important issue. However, I thought this place could be reviewed in case it is re-visited.

[ulucinar]

Thanks for reporting this unwanted behavior. I've tackled with this issue a bit and found this upstream issue regarding the floating comments when we modify the existing code. Let me open a separate issue to address this as I suspect we would need to properly calculate the position of that single comment in the transformer source code or use a higher level library like this one to fix this and currently, to the best of my knowledge, it does cause any functional issues.

[sergenyalcin]

Will this statement cause any issues with the Azure-ResourceGroup resource? Because the API group of ResourceGroup is the same as the apiGroupSuffix: azure.upbound.io`.

I mean, the split statement will be this: github.com/upbound/provider-azure/apis/azure/v1beta1

Then, the calculated group will be azure.azure.upbound.io.

[ulucinar]

Thanks @sergenyalcin, good catch indeed! Given this a try with upbound/provider-azure as you suggested and what we generate is:

m, l, err = apisresolver.GetManagedResource("azure.azure.upbound.io", "v1beta1", "ResourceGroup", "ResourceGroupList")
		if err != nil {
			return errors.Wrap(err, "failed to get the reference target managed resource and its list for reference resolution")
		}

Added the --api-group-override command-line option (shorthand -o) so that any API group overrides can be configured. Also added the --api-resolver-package (shorthand -a) command-line option so that we can properly configure the API resolver function's (GetManagedResource) package.

So, when we run the transformer with the following command-line options:

go run github.com/crossplane/upjet/cmd/resolver -g azure.upbound.io -a github.com/upbound/provider-azure/internal/apis -o azure.azure.upbound.io=azure.upbound.io

what gets now generated is:

		m, l, err = apisresolver.GetManagedResource("azure.upbound.io", "v1beta1", "ResourceGroup", "ResourceGroupList")
		if err != nil {
			return errors.Wrap(err, "failed to get the reference target managed resource and its list for reference resolution")
		}

In a future iteration (especially if some other providers also adopt this transformer), we may also generate the GetManagedResource function via upjet for the provider and remove the --api-resolver-package option.

[sergenyalcin]

Is there the above problem here?

[ulucinar]

Yes, we need to address this also. Please see the above comment explaining group overrides configuration and the test with upbound/provider-azure. Thank you @sergenyalcin.

[sergenyalcin]

Thanks @ulucinar LGTM!