Cluster-api: KubeadmConfig conversion webhook cannot return errors when downconverting

[ncdc]

What steps did you take and what happened:

• Create a v1alpha3 KubeadmConfig with status.dataSecretName set
• Retrieve it as v1alpha2 (so it's converted)
• Get an error

What did you expect to happen:

• No error

Anything else you would like to add:
I checked with sig-apimachinery and conversion cannot fail because an older api version doesn't have the same fields as a newer one. In Convert_v1alpha3_KubeadmConfigStatus_To_v1alpha2_KubeadmConfigStatus, we're returning an error if the a3 version has status.dataSecretName. We can choose from:

1. Silently drop the field
2. Add the new field to all API versions so that it's convertible (it would be present but unreconciled/unused in older versions)
3. Shuttle the field through metadata (annotation) so that a3 -> a2 -> a3 would preserve the value.

Option 1 is probably a nonstarter because it would mean we'd lose data if we did a3 -> a2 -> update apiserver.

Environment:

• Cluster-api version: master

/kind bug

[ncdc]

@vincepri

[vincepri]

/assign

[vincepri]

CC @detiber given that failing conversion was suggested during review. I don’t have enough experience with conversions, I am ok with whatever is correct

[ncdc]

Slack conversation w/Jordan: https://kubernetes.slack.com/archives/C0EG7JC6T/p1576874571057600

[ncdc]

"conversion is not allowed to fail for reasons like that"

[detiber]

My suggestion for failing downconversion was specifically because we weren't tackling the problem of preserving the data on downconversion...

Will we hit any issues with Option 2 if someone is using an old version of v1alpha2 APIs vendored into a client application?

[ncdc]

Will we hit any issues with Option 2 if someone is using an old version of v1alpha2 APIs vendored into a client application?

If they do an update instead of a patch, yes. That's why we've been pushing patch so heavily. Sounds like the annotation approach is the safest?

[detiber]

If they do an update instead of a patch, yes. That's why we've been pushing patch so heavily. Sounds like the annotation approach is the safest?

I did a quick bit of prototyping a while back to prove out the idea, and it was relatively simple to implement, just had to implement it in either the ConvertFrom methods, or in the Convert_v1alpha3_<Type>_To_v1alpha2_Type> methods, since the Spec/Status conversion methods don't have access to the annotations.

[vincepri]

Seems like an annotation on the v1a2 types is the way to go?

[ncdc]

Yes

[vincepri]

on it

[vincepri]

/lifecycle active

[vincepri]

Any preferences regarding the annotation name?

[detiber]

<type>.cluster.x-k8s.io/compat-<fieldpath1>.<fieldpath2>...?

[vincepri]

I was more leaning towards marshaling the entire object and avoid any dynamic annotation, wdyt?

[detiber]

@vincepri wouldn't that introduce complications on how to properly merge the resources during a later conversion?

[vincepri]

What do you mean? We wouldn't just unmarshal the entire object in the destination, we'll use the information saved to convert only fields that need this kind of manual conversion.

[ncdc]

This is fixed.
/close

[k8s-ci-robot]

@ncdc: Closing this issue.

In response to this:

This is fixed.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.