conversion-gen should be able to use conversion functions defined in other packages

[ncdc]

Let's say I have the following package structure:

• a1
  • types.go (Widget)
  • sub1
    • types.go (Fromble)
• a2
  • types.go (Widget)
  • sub2
    • types.go (Fromble)

and types like

// a1
type Widget struct {
  f sub1.Fromble
}

// a2
type Widget struct {
  f sub2.Fromble
}

I'd like to be able to generate conversion functions for sub1/sub2. Then I'd like to generate conversion functions for a1/a2 and have them use the generated conversion functions from sub1/sub2. I haven't found a way to do this. I've trying using --extra-peer-dirs, but all the generated files are currently using the default build tag (ignore_autogenerated), and gengo purposefully excludes these files:

https://github.com/kubernetes/gengo/blob/e0e292d8aa122d458174e1bef5f142b4d0a67a05/args/args.go#L137-L138

I could potentially use a unique tag per package & generator, but I have a real world example where some of the packages come from other repositories, and it's not easy to change their tags.

Is there a way to make this work with conversion-gen as-is, or will it require changes, either to conversion-gen or gengo?

cc @sttts @vincepri @yastij

[ncdc]

@wojtek-t I think you originally added the code to gengo that excludes files containing the build tag from being processed (it was several years ago). Know if there's a workaround, or should we consider making it configurable in gengo?

cc @smarterclayton @deads2k @liggitt @lavalamp

[yastij]

making it configurable makes sense either by pre-computing names or providing file names externally

[drekle]

If I understand the issue correctly I have also hit this a couple of times. I believe the typecasting below is a workaround? (however it assumes you use the new type throughout your codebase)

// a1
type a1fromble sub1.Fromble

type Widget struct {
  f a1fromble
}

// a2
type a2fromble sub2.Fromble

type Widget struct {
  f a2fromble
}

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[vincepri]

/lifecycle frozen

[lavalamp]

Sorry first time I saw this... I don't actually understand what result you're hoping for. You have 4 separate things and invoking conversion gen twice? Are you hoping that this will result in (4-1)^2 conversion functions such that the scheme can turn any of your 4 things into any of the others?

[ncdc]

Ugh, now I have to remember why I wrote this 😛. I definitely had a good reason for it... It's for Cluster API, and IIRC we have one API package that includes types from another API package. And then we have 2 different API versions. So this was wanting to be able to call Convert_v1alpha1_Widget_to_v1alpha2_Widget and have it use conversion functions from the "sub" packages (which are valid apimachinery-ish API packages), which isn't currently possible.

[ncdc]

@lavalamp here's a more concrete, specific example.

In Cluster API, we have two separate API groups:

• bootstrap
• controlplane

In the bootstrap API group, we have a KubeadmConfigSpec type.

In the controlplane API group, we have a KubeadmControlPlaneSpec type, and this has a bootstrap.KubeadmConfigSpec field.

We previously only had a single version for the controlplane API group - v1alpha3. We are now introducing new versions for all our API groups, meaning we want to have:

• bootstrap
  • v1alpha3
  • v1alpha4
• controlplane
  • v1alpha3, referencing bootstrap.v1alpha3 types
  • v1alpha4, referencing bootstrap.v1alpha4 types

We need to be able to convert a KubeadmControlPlaneSpec between v1alpha3 and v1alpha4. In doing so, we need to be able to convert between bootstrap.KubeadmConfigSpec v1alpha3 and v1alpha4. When we run conversion-gen, we get output like this:

func autoConvert_v1alpha3_KubeadmControlPlaneSpec_To_v1alpha4_KubeadmControlPlaneSpec(in *KubeadmControlPlaneSpec, out *v1alpha4.KubeadmControlPlaneSpec, s conversion.Scope) error {
	out.Replicas = (*int32)(unsafe.Pointer(in.Replicas))
	out.Version = in.Version
	out.InfrastructureTemplate = in.InfrastructureTemplate
	// TODO: Inefficient conversion - can we improve it?
	if err := s.Convert(&in.KubeadmConfigSpec, &out.KubeadmConfigSpec, 0); err != nil {
		return err
	}
	out.UpgradeAfter = (*v1.Time)(unsafe.Pointer(in.UpgradeAfter))
	out.NodeDrainTimeout = (*v1.Duration)(unsafe.Pointer(in.NodeDrainTimeout))
	return nil
}

(and similar for the other direction)

We are using controller-runtime, and its conversion conventions/support has us implement functions that look like this:

func (src *KubeadmControlPlane) ConvertTo(destRaw conversion.Hub) error {
	dest := destRaw.(*v1alpha4.KubeadmControlPlane)
	return Convert_v1alpha3_KubeadmControlPlane_To_v1alpha4_KubeadmControlPlane(src, dest, nil)
}

func (dest *KubeadmControlPlane) ConvertFrom(srcRaw conversion.Hub) error {
	src := srcRaw.(*v1alpha4.KubeadmControlPlane)
	return Convert_v1alpha4_KubeadmControlPlane_To_v1alpha3_KubeadmControlPlane(src, dest, nil)
}

The nil for the 3rd parameter to both the Convert_* functions is a conversion.Scope. I filed https://github.com/kubernetes-sigs/controller-runtime/issues/810 in controller-runtime but haven't made any progress on that front (how do we create/get a conversion.Scope?).

We do have conversion functions for all the types involved, but because controlplane.KubeadmControlPlaneSpec and bootstrap.KubeadmConfigSpec are in two different packages, conversion-gen doesn't have a way to find & use the conversion functions from another package (or if it can, I haven't figured out the right incantation).

Does this help explain what we're trying to do?

[ncdc]

And to repeat from the original description, because our generated files have the standard/default ignore_autogenerated build tag, with https://github.com/kubernetes/gengo/blob/e0e292d8aa122d458174e1bef5f142b4d0a67a05/args/args.go#L137-L138, the conversion functions in the bootstrap package are ignored, because they're in a file that has // +build !ignore_autogenerated.

[thockin]

https://github.com/kubernetes/kubernetes/pull/123736#issue-2170280496

[sbueringer]

Seems like in Cluster API we were able to use conversion functions from other packages after all by:

• setting different build-tags for different API packages: https://github.com/kubernetes-sigs/cluster-api/blob/main/Makefile#L452
• setting extra-peer-dirs: https://github.com/kubernetes-sigs/cluster-api/blob/main/Makefile#L514-L518 (I think this should have been extra-dirs though instead of extra-peer-dirs)

(we have been doing this for years, I just didn't know about why we are setting the build tags)

Now when I tried to bump to conversion-gen v0.30.0 this doesn't work anymore because the build-tag flag was dropped. (I assume that is the root cause)

So as far as I can tell the --extra-dirs flag doesn't work as expected.

I now worked around it by adding "local" conversion functions pointing to the other API package, e.g.:

func Convert_v1beta1_KubeadmConfigSpec_To_v1alpha3_KubeadmConfigSpec(in *bootstrapv1.KubeadmConfigSpec, out *bootstrapv1alpha3.KubeadmConfigSpec, s apiconversion.Scope) error {
	return bootstrapv1alpha3.Convert_v1beta1_KubeadmConfigSpec_To_v1alpha3_KubeadmConfigSpec(in, out, s)
}

For us the workaround is absolutely fine, I mostly wanted to surface this issue because it worked before. Maybe I"m also just missing how to configure this correctly

For reference, the PR to bump Cluster API to conversion-gen v0.30.0: https://github.com/kubernetes-sigs/cluster-api/pull/10474