Expose conversion option to inject key/values in the conversion to list

[turkenh]

Description of your changes

We have observed some issue with the EKS Cluster resource where when we apply the manifest and let it to resolve references to spec.forProvider.vpcConfig.securityGroupIds, we noticed the following error in the status:

  - lastTransitionTime: "2025-02-06T07:28:46Z"
    message: 'cannot patch the managed resource via server-side apply: Cluster.eks.aws.upbound.io
      "some-eks-cluster" is invalid: [spec.forProvider.vpcConfig: Invalid
      value: "null": spec.forProvider.vpcConfig in body must be of type array: "null",
      <nil>: Invalid value: "null": some validation rules were not checked because
      the object was invalid; correct the existing errors to complete validation]'
    reason: ReconcileError
    status: "False"
    type: Synced

Our assumption (which is validated by observing this changes fixes the problem) is as follows:

Server Side Apply uses merge strategies to make decisions during merging changes by various owners/managers. In EKS Cluster object, for vpcConfig field, we mark +listType as map and //+listMapKey as index. During conversion between v1beta1 to v1beta2, we convert the that field from array to object, losing the index field since it is not in the schema. This is fine in most cases since in v1beta1 of the object schema index defaults to "0" , even though you don’t provide it.

However, with Server Side Apply, apparently some on the fly conversions happening when different managers using different api versions and losing index field causing unexpected merging results and drop of the whole spec.forProvider.vpcConfig object.

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

Unit tests and a custom build for provider-aw-eks with this change.

Here is the reproducer for the original issue: https://github.com/turkenh/upjet-pr-465-reproducer

[sergenyalcin]

@turkenh Firstly, thank you very much for your very valuable effort and handling this. And also thank you for your time that you spent resolving this issue🙏🏼. Your solution addresses the problem effectively and makes a lot of sense to me.

nit: This is not a blocker. I just wanted to note an alternative for future consideration.

From a design perspective, I was wondering about the possibility of making ConvertOptions available in a more generic way. Currently, we have a specific definition and are modifying the Convert function for this case, which leads to resource/provider-specific business logic being implemented on the upjet side. If we think of ConvertOptions as an interface, it might offer a chance to configure it through a broader and more generic API. This could also help decouple resource/provider-specific logic from upjet (although how exactly to achieve this requires further thought—just a high-level comment).

That said, I’m not fully convinced that this level of generalization is necessary, since it’s unclear how often we’ll encounter similar cases or what other custom configurations might be needed in the future.

Once again, thank you for your effort—it solves the problem effectively. LGTM!

[turkenh]

@turkenh Firstly, thank you very much for your very valuable effort and handling this. And also thank you for your time that you spent resolving this issue🙏🏼. Your solution addresses the problem effectively and makes a lot of sense to m

❤️ 🙇

Currently, we have a specific definition and are modifying the Convert function for this case, which leads to resource/provider-specific business logic being implemented on the upjet side. If we think of ConvertOptions as an interface, it might offer a chance to configure it through a broader and more generic API. This could also help decouple resource/provider-specific logic from upjet (although how exactly to achieve this requires further thought—just a high-level comment).

Not really. Currently, we don't have resource/provider specific logic embedded into upjet. This PR exposes an option to inject in the SingletonListConverter, with a non breaking change by introducing an optional argument (opts) to the NewSingletonListConversion function. So, logic still lives on the provider side per resource, see the example PR on provider side. At first, I went one step further and introduced this option in the config.Resource struct, enabling overriding this under config/eks/config.go just like the other configuration, but later reverted that since this is only needed by a single resource right now and no need to generalize further. Another advantage is this implementation is closer to conversion initialization. Happy to explain my thought process further if this still doesn't makes sense :)