unexpected behavior for bool type with api conversion

[morvencao]

Issue Description:

We are supporting Multi-Version APIs for our controller built with kubebuilder, we have API of two versions: v1beta1 and v1beta2, the api conversion is done through the webhook. The v1beta2 acts as hub version(storage version), while v1beta1 acts as spoke version,

In v1beta1 and v1beta2 versions, we have some fields of bool type, we found unexpected behavior with these fields.

Case 1: for a field EnableMetrics exists in both v1beta1 and v1beta2 versions, its default value in both v1beta1 and v1beta2 versions is true.

If we set field EnableMetrics in v1beta1 version to false explicitly, then the field EnableMetrics in v1beta1 will converted to v1beta2 and that is expected from conversion logs.

However then we see logs of conversion from v1beta2 to v1beta1, at that time the field EnableMetrics in v1beta2 actually becomes true, then the field EnableMetrics in v1beta1 also become true after conversion.

We suspect that the default value of field EnableMetrics in v1beta2 overrided the user specified value(false), and we're sure we don't set the value in our controller.

See the conversion logs for more details:

2021-03-25T09:04:34.269Z INFO  api_conversion  Converting v1beta1 to v1beta2 {"EnableMetrics in v1beta1": false}
2021-03-25T09:04:34.282Z INFO  api_conversion  Converting v1beta2 to v1beta1 {"EnableMetrics in v1beta2": true}
2021-03-25T09:04:34.286Z INFO  api_conversion  Converting v1beta2 to v1beta1 {"EnableMetrics in v1beta2": true}
2021-03-25T09:04:34.290Z INFO  api_conversion  Converting v1beta2 to v1beta1 {"EnableMetrics in v1beta2": true}
2021-03-25T09:04:34.299Z INFO  api_conversion  Converting v1beta2 to v1beta1 {"EnableMetrics in v1beta2": true}

Expected Behavior:

The field EnableMetrics should not be override with default value in v1beta2, since we have explicitly set it to false in v1beta1.

What versions of software are you using? Specifically, the following are often useful:

• go version 1.15
• kubebuilder version (kubebuilder version) and scaffolding version (check your PROJECT file)

...
layout: go.kubebuilder.io/v3
version: 3-alpha
...

• controller-runtime version (check your go.mod file). v0.8.0
• controller-tools version v0.4.1
• Kubernetes & kubectl versions (just run kubectl version against your API server)

# kubectl version
Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.0", GitCommit:"af46c47ce925f4c4ad5cc8d1fca46c7b77d13b38", GitTreeState:"clean", BuildDate:"2020-12-08T17:59:43Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.0+e49167a", GitCommit:"e49167aad6a08046be6ab21ff13029110c76951d", GitTreeState:"clean", BuildDate:"2021-01-28T07:35:27Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

/kind bug

@camilamacedo86 Would you help on this?

[camilamacedo86]

Hi @morvencao,

It does not show a bug with Kubebuilder CLI tool. I understand that you are looking for help/support over how to use the webhooks conversion and implement it.

I think @varshaprasad96 has been doing contributions on that and might help you here. @varshaprasad96 could you please give a hand on this one?

[Adirio]

You have not provided kubebuilder version.

Also, are you using kubebuilder or operator-sdk?

• Project version 3-alpha was something temporal that has never made it into a release. Operator SDK used it as it required new features that weren't present in any release so it was using nightly builds of k9r. Operator SDK offers a tool to migrate this project files to version 3, by the way.
• No plugin of k9r supports controller-runtime v0.8.0 yet.
  These points make me thing you are actually using operator-sdk and not kubebuilder directly.

From kubebuilder we don't implement any conversion webhook logic yet, you need to implement ConvertTo and ConvertFrom functions for the type yourself. I don't know if operator-sdk does something extra for conversion webhooks.

[camilamacedo86]

SDK does not that. @varshaprasad96 has been working on in the PR to scaffold a code to help users with. See: #2084

All points raised by @Adirio are valid. Would also recommend to you use the latest beta version and do the migration. See: https://master.book.kubebuilder.io/migration/v2vsv3.html

And then, was decided to just bump the controller-runtime to 0.8.0 for the 3.1.0 release which will be done sequentially after the 3.0.0 stable one which is very closer to happens. See the pr: #1961

However, has no breaking changes from the controller-runtime version used on master now and 0.8.0 so it would also be fine.

[morvencao]

thanks @camilamacedo86 @Adirio
We do use the operator-sdk to initialize our project, but operator-sdk does not include the doc for supporting Multi-Version APIs, but kubebuilder does.

We have implemented logic of ConvertTo and ConvertFrom functions for our type, the conversion for the other fields are working as expected, except the fields of bool type.

I believe this should be relevant to the defaulting feature introduced by the the CustomResourceDefinition of apiextensions.k8s.io/v1, see: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/_print/#defaulting

I'm not sure if the kubebuilder Multi-Version API/API conversion supports the defaulting feature of CRD.

[Adirio]

Without the code of those conversions, or at least a simplified version, there is little we can do to check that. Providing a minimal reproducible example would be the best approach.

[morvencao]

@Adirio Before I logged the issue, I did some experiment.
The results show that without conversion, the defaulting feature of CRD works as expected.
That said, if I apply a CR with EnableMetrics to be false, then I will get it again, the field is still false, so it indicates that fields isn't overrided by the default value?

[Adirio]

I think it may be related to some omitempty tag which is making it not parse the false value (as it is the zero value for booleans) and then the default webhook is kicking as it doesn't see any value for that field. Can you post the code of the conversion functions (at least a simplified example)?

[morvencao]

Maybe, I'll try to verify if it's the omitempty tag.

The conversion logic is quite simple:

func (src *MCO) ConvertTo(dstRaw conversion.Hub) error {
	dst := dstRaw.(*v1beta2.MCO)

	dst.Spec.RetentionConfig = &v1beta2.RetentionConfig{
		RetentionResolutionRaw: src.Spec.RetentionResolutionRaw,
		RetentionResolution5m:  src.Spec.RetentionResolution5m,
		RetentionResolution1h:  src.Spec.RetentionResolution1h,
	}

	dst.Spec.EnableMetrics = src.Spec.EnableMetrics

	/*
		The rest of the conversion is pretty rote.
	*/
	// ObjectMeta
	dst.ObjectMeta = src.ObjectMeta

	// Spec
	dst.Spec.ImagePullPolicy = src.Spec.ImagePullPolicy
	dst.Spec.ImagePullSecret = src.Spec.ImagePullSecret
	dst.Spec.NodeSelector = src.Spec.NodeSelector
	dst.Spec.Tolerations = src.Spec.Tolerations

	// Status
	dst.Status.Conditions = src.Status.Conditions

	// +kubebuilder:docs-gen:collapse=rote conversion
	return nil
}

/*
ConvertFrom is expected to modify its receiver to contain the converted object.
Most of the conversion is straightforward copying, except for converting our changed field.
*/

// ConvertFrom converts from the Hub version (v1beta2) to this version.
func (dst *MCO) ConvertFrom(srcRaw conversion.Hub) error {
	src := srcRaw.(*v1beta2.MCO)

	dst.Spec.RetentionResolutionRaw = src.Spec.RetentionConfig.RetentionResolutionRaw
	dst.Spec.RetentionResolution5m = src.Spec.RetentionConfig.RetentionResolution5m
	dst.Spec.RetentionResolution1h = src.Spec.RetentionConfig.RetentionResolution1h

	dst.Spec.EnableMetrics = src.Spec.EnableMetrics

	/*
		The rest of the conversion is pretty rote.
	*/
	// ObjectMeta
	dst.ObjectMeta = src.ObjectMeta

	// Spec
	dst.Spec.ImagePullPolicy = src.Spec.ImagePullPolicy
	dst.Spec.ImagePullSecret = src.Spec.ImagePullSecret
	dst.Spec.NodeSelector = src.Spec.NodeSelector
	dst.Spec.Tolerations = src.Spec.Tolerations

	// Status
	dst.Status.Conditions = src.Status.Conditions

	// +kubebuilder:docs-gen:collapse=rote conversion
	return nil
}

[Adirio]

That's strange, both methods are setting that variable directly. Any mutating webhook?

[morvencao]

@Adirio No, I don't enable mutating and validate webhook.

[morvencao]

@Adirio Looks like removing the omitempty tag resolves the issue. You're right, the false value for boolean type is not parsed correctly. I'll do more verification. Thanks a lot!

[morvencao]

Removing the omitempty tag really mitigate the issue, but I have to mention that for the v1beta1 objects already exist in the APIServer before we introduce the API conversion webhook, because those serialized objects(yaml/json) were marshalled with omitempty. We're still trying to resolve this.