unexpected behavior for bool type with api conversion #2109

morvencao · 2021-03-25T10:03:04Z

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?

The text was updated successfully, but these errors were encountered:

camilamacedo86 · 2021-03-25T10:39:22Z

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 · 2021-03-25T10:40:04Z

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 · 2021-03-25T10:47:16Z

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 · 2021-03-25T11:02:16Z

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 · 2021-03-25T11:08:57Z

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 · 2021-03-25T11:12:45Z

@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 · 2021-03-25T11:17:12Z

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 · 2021-03-25T11:44:34Z

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 · 2021-03-25T12:20:20Z

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

morvencao · 2021-03-25T12:37:06Z

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

morvencao · 2021-03-25T15:16:21Z

@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 · 2021-03-26T02:51:07Z

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.

fejta-bot · 2021-06-24T03:34:02Z

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-contributor-experience at kubernetes/community.
/lifecycle stale

k8s-triage-robot · 2021-07-26T19:14:41Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

k8s-triage-robot · 2021-08-25T20:12:47Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2021-08-25T20:12:54Z

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/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.

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 25, 2021

camilamacedo86 added kind/support Categorizes issue or PR as a support question. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 25, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2021

prometherion mentioned this issue Jul 17, 2021

Missing migration of enable-node-ports annotation from v1alpha1 to v1beta1 projectcapsule/capsule#342

Closed

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 26, 2021

k8s-ci-robot closed this as completed Aug 25, 2021

irinamihai mentioned this issue Feb 8, 2022

Fix issues openshift-kni/cluster-group-upgrades-operator#73

Merged

hrak mentioned this issue Mar 14, 2023

Add flag to allow userdata to be uncompressed kubernetes-sigs/cluster-api-provider-cloudstack#216

Merged

AntonAleksandrov13 mentioned this issue Dec 8, 2023

Unexpected CR conversion behavior #3705

Closed

helgebonert mentioned this issue Dec 12, 2023

fix: remove omitempty tag for boolean leaveOnDelete urbanmedia/passbolt-operator#126

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unexpected behavior for bool type with api conversion #2109

unexpected behavior for bool type with api conversion #2109

morvencao commented Mar 25, 2021 •

edited

Loading

camilamacedo86 commented Mar 25, 2021

Adirio commented Mar 25, 2021 •

edited

Loading

camilamacedo86 commented Mar 25, 2021

morvencao commented Mar 25, 2021 •

edited

Loading

Adirio commented Mar 25, 2021 •

edited

Loading

morvencao commented Mar 25, 2021

Adirio commented Mar 25, 2021

morvencao commented Mar 25, 2021 •

edited

Loading

Adirio commented Mar 25, 2021

morvencao commented Mar 25, 2021

morvencao commented Mar 25, 2021

morvencao commented Mar 26, 2021 •

edited

Loading

fejta-bot commented Jun 24, 2021

k8s-triage-robot commented Jul 26, 2021

k8s-triage-robot commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

unexpected behavior for bool type with api conversion #2109

unexpected behavior for bool type with api conversion #2109

Comments

morvencao commented Mar 25, 2021 • edited Loading

Issue Description:

Expected Behavior:

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

camilamacedo86 commented Mar 25, 2021

Adirio commented Mar 25, 2021 • edited Loading

camilamacedo86 commented Mar 25, 2021

morvencao commented Mar 25, 2021 • edited Loading

Adirio commented Mar 25, 2021 • edited Loading

morvencao commented Mar 25, 2021

Adirio commented Mar 25, 2021

morvencao commented Mar 25, 2021 • edited Loading

Adirio commented Mar 25, 2021

morvencao commented Mar 25, 2021

morvencao commented Mar 25, 2021

morvencao commented Mar 26, 2021 • edited Loading

fejta-bot commented Jun 24, 2021

k8s-triage-robot commented Jul 26, 2021

k8s-triage-robot commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

morvencao commented Mar 25, 2021 •

edited

Loading

Adirio commented Mar 25, 2021 •

edited

Loading

morvencao commented Mar 25, 2021 •

edited

Loading

Adirio commented Mar 25, 2021 •

edited

Loading

morvencao commented Mar 25, 2021 •

edited

Loading

morvencao commented Mar 26, 2021 •

edited

Loading