Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Objectmeta of embedded object goes missing during CRD conversion #448

Closed
champak opened this issue May 29, 2020 · 36 comments
Closed

Objectmeta of embedded object goes missing during CRD conversion #448

champak opened this issue May 29, 2020 · 36 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.

Comments

@champak
Copy link

champak commented May 29, 2020

Hi folks,
I ran into an odd issue with CRD conversion using controller-tools v0.2.4/kubebuilder 2.2.0/apiserver 17.1 This CRD has embedded native specs like the PodTemplateSpec in its spec. I found that in the conversion functions the ObjectMeta of the embedded PodTemplateSpec is nulled out. Other attributes look OK. There is nothing in the validation callback. I had needed to add preserveUnknownFields: false to the CRD to be able to deploy the yaml; not sure if that has some sideffect

The ObjectMeta for the CRD itself shows up fine. It is the embedded spec's ObjectMeta that seemed to be affected. Does this sound like any known issue ? One that looked somewhat related from a search was #279 with comments from @DirectXMan12 but was not quite the same problem.
Thanks for any insight !

@DirectXMan12
Copy link
Contributor

/kind bug
/priority critical-urgent
/needs-more-info

any idea if this is happening during the conversion webhook? Can you post a reproducer?

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jun 1, 2020
@champak
Copy link
Author

champak commented Jun 2, 2020

I had added logs to the ConvertTo(dstRaw conversion.Hub) and ConvertFrom(srcRaw conversion.Hub) functions in the file api/v<>/<>_conversion.go to display the contents of the ObjectMeta from the CRD and the embedded PodTemplateSpec. The ObjectMeta.Name etc from the CRD level had the expected contents but the contents from the embedded podtemplatespec were nulled. Are there other places that may be useful to log ? I was wondering if the api-server itself could have chosen to not send the metadata of the embedded spec to the webhook. Have not looked yet at what all layers are in play between the api-server and the ConvertTo/From functions. I am wondering if I am missing some knob. I was following the kubebuilder book on the conversion.

The CRD structure is on the lines of ;
CRDfooSpec {
barspec
}
barspec {
Pod corev1.PodTemplateSpec ...
...
}

The embedded podtemplate spec contents are visible in the last-applied-configuration annotation of the CRD so the info did make it to the api-server. I will try to post a repro. Thanks !

@champak
Copy link
Author

champak commented Jun 2, 2020

I created a simple repro for this issue. @DirectXMan12 please see https://github.com/champak/repro-kbuilder-version-issue .

If the sample yaml at https://github.com/champak/repro-kbuilder-version-issue/blob/master/config/samples/left_v1alpha1_repro.yaml

is applied, the logs at the webhook converter look like

2020/06/02 19:42:14 ConvertFrom : working on repro-sample 
2020/06/02 19:42:14 ConvertFrom working on Pod  attr bar  ===> I tried printing the pod name; nothing shows up.
2020/06/02 19:42:30 ConvertFrom : working on repro-sample 
2020/06/02 19:42:30 ConvertFrom working on Pod  attr bar 

The name of the CRD "repro-sample" show up. The name of the Pod does not. The logs are from api/v1/repro_conversion.go. The versions are going between v1alpha1 and v1. I followed the kubebuilder scaffolds and book.

If logs elsewhere would tell us more send a ptr. Thanks for looking !

@champak
Copy link
Author

champak commented Jun 2, 2020

And a get on the CRD looks like

$ kctl get repro.left.play.zone/repro-sample -o yaml | more
apiVersion: left.play.zone/v1
kind: Repro
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"left.play.zone/v1alpha1","kind":"Repro","metadata":{"annota
tions":{},"name":"repro-sample","namespace":"default"},"spec":{"foo":"bar","grap
h":{"dummy":"isthere","pod":{"metadata":{"name":"mia-pod-name"},"spec":{"contain
ers":[{"image":"busybox","name":"a-container"}]}}}}}
  creationTimestamp: "2020-06-02T19:42:30Z"
  generation: 1
  name: repro-sample
  namespace: default
  resourceVersion: "22387679"
  selfLink: /apis/left.play.zone/v1/namespaces/default/reproes/repro-sample
  uid: fac7fae5-3592-4925-a454-69a98e20e1bf
spec:
  foo: bar
  graph:
    dummy: ""
    pod:
      metadata: {}
      spec:
        containers: null
status: {}

The pod metadata is missing. Similar behavior for embedding other native types like PVCs or services. Perhaps I am missing some setting but found nothing yet.

@champak
Copy link
Author

champak commented Jun 4, 2020

@DirectXMan12 let me know if I try to get any other details etc. that may help. I hope the example provides enough info. One thing I had noted is that the meta-data was missing even before we hit the convert functions. For the v1alpha1 CR version we hit the reconcile loop before going to the convert callbacks and the bits are already missing. That made me wonder if it is the apiserver that may have pruned the info due to some setting. The operator initialization log had

2020-06-04T19:15:50.982Z	INFO	controller-runtime.metrics	metrics server is starting to listen	{"addr": "127.0.0.1:8080"}
2020-06-04T19:15:50.982Z	INFO	controller-runtime.builder	skip registering a mutating webhook, admission.Defaulter interface is not implemented	{"GVK": "left.play.zone/v1alpha1, Kind=Repro"}
2020-06-04T19:15:50.982Z	INFO	controller-runtime.builder	skip registering a validating webhook, admission.Validator interface is not implemented	{"GVK": "left.play.zone/v1alpha1, Kind=Repro"}
2020-06-04T19:15:50.982Z	INFO	controller-runtime.webhook	registering webhook	{"path": "/convert"}
2020-06-04T19:15:50.982Z	INFO	controller-runtime.builder	conversion webhook enabled	{"object": {"metadata":{"creationTimestamp":null},"spec":{"graph":{"pod":{"metadata":{"creationTimestamp":null},"spec":{"containers":null}},"dummy":""}},"status":{}}}
2020-06-04T19:15:51.080Z	INFO	setup	starting manager
2020-06-04T19:15:51.082Z	INFO	controller-runtime.manager	starting metrics server	{"path": "/metrics"}
2020-06-04T19:15:51.182Z	INFO	controller-runtime.webhook.webhooks	starting webhook server
2020-06-04T19:15:51.183Z	INFO	controller-runtime.certwatcher	Updated current TLS certificate
2020-06-04T19:15:51.185Z	INFO	controller-runtime.webhook	serving webhook server	{"host": "", "port": 9443}
2020-06-04T19:15:51.186Z	INFO	controller-runtime.certwatcher	Starting certificate watcher
2020-06-04T19:16:08.575Z	DEBUG	controller-runtime.manager.events	Normal	{"object": {"kind":"ConfigMap","namespace":"metarepro-system","name":"controller-leader-election-helper","uid":"9bb0c9fc-a56d-4436-9a62-57329c1a3e77","apiVersion":"v1","resourceVersion":"22876473"}, "reason": "LeaderElection", "message": "metarepro-controller-manager-5575c95b5c-c9n48_6428cfcb-da12-4e36-8a6b-9d1e6e31aff6 became leader"}
2020-06-04T19:16:08.576Z	INFO	controller-runtime.controller	Starting EventSource	{"controller": "repro", "source": "kind source: /, Kind="}
2020-06-04T19:16:08.877Z	INFO	controller-runtime.controller	Starting Controller	{"controller": "repro"}
2020-06-04T19:16:08.977Z	INFO	controller-runtime.controller	Starting workers	{"controller": "repro", "worker count": 1}

@champak
Copy link
Author

champak commented Jun 9, 2020

@DirectXMan12 please let me know if you need any other info to take a look into this issue. Thanks !

@DirectXMan12
Copy link
Contributor

sorry. Did some digging thursday / friday. I think you're hitting unknown field elision -- need to confirm that, but I think it's because conversion requires preserveUnknownFields: false, which will strip fields from the under-specified ObjectMeta. Related: #385, #395

@champak
Copy link
Author

champak commented Jun 11, 2020

@DirectXMan12 Thanks for taking a look ! If there is any hack that would allow crd conversion to work w/o needing preserveUnknownFields: false let me know and I can try it out. I could not tell if the changes from #385, #395 will get pulled into controller-gen main. I did not follow what functional need makes preserveUnknownFields: false mandatory. I can see it is desirable since dealing with conversion of unstructured data is ill-defined. But in many cases we merely want the unstructured blob to flow over to the new CRD version without changes. Would be nice to have a means for that.

@DirectXMan12
Copy link
Contributor

kubernetes itself requires preserveUnknownFields: false if conversion is enabled for v1beta1, and IIRC has it on by default in v1

@champak
Copy link
Author

champak commented Jun 11, 2020

@DirectXMan12 thanks for explaining ; in that case it sounds like something on the lines of #395 would be needed. I did not see updates on the issue since Jan ; is a different approach under consideration ? Think it would be essential to preserve at least a few common fields from ObjectMeta like name, namespace to be able to make embeddable native resources work well in a versioned CRD. Base types like replicasets embed podspecs and can handle version changes. Would be awesome to be able to do that with CRDs as well.

@champak
Copy link
Author

champak commented Jun 18, 2020

@DirectXMan12 is there any possibility of a fix or workaround for this issue in the near future ? Thanks.

@champak
Copy link
Author

champak commented Jul 8, 2020

@DirectXMan12 I see the review for #395 is still open. Any chance it will be available soon ? Else are there other options for getting around this issue ? Thanks

@champak
Copy link
Author

champak commented Aug 19, 2020

@DirectXMan12 @droot are the changes on #395 planned for any upcoming release ? There does not seem to be a reasonable workaround. Using a custom version of ObjectMeta is not an option for anything that is already out there. Was hoping there would be a fix or the changes on #395 will get merged.

@DirectXMan12
Copy link
Contributor

@champak I've commented on #395. I think it's pretty close to being good to go, it just needs a tweak. Sorry about dropping the ball on that one. IDK if the original author is still around, but if you have some bandwidth and want to pick it up, feel free.

@champak
Copy link
Author

champak commented Sep 15, 2020

@DirectXMan12 Thanks for getting back. I shall give it a shot. Were you saying we need a crd option for controller-gen on the lines of trivialVersions or maxDescLen to turn this on ?

@DirectXMan12
Copy link
Contributor

Were you saying we need a crd option for controller-gen on the lines of trivialVersions or maxDescLen to turn this on ?

Yeah, exactly.

Note that @sttts has a counter-proposal, so might be worth reconciling that (pun intended ;-) )

@champak
Copy link
Author

champak commented Sep 26, 2020

@DirectXMan12 I added a simple option for using #395 (see above). Pl take a look when you can and let me know what it needs. I could not find a decent way to test with a private version of controller-gen. The go get mechanism in the kubebuilder makefile want to install a published version. If you send a ptr on how to test it out shall run thru it. Thanks.

@champak
Copy link
Author

champak commented Oct 1, 2020

@DirectXMan12 just polling on this issue...
On a separate topic, how do we build a local controller-gen binary from the repo. Using the packaged test works but was wondering if there is some way to build the binary directly.

@DirectXMan12
Copy link
Contributor

On a separate topic, how do we build a local controller-gen binary from the repo

go build ./cmd/controller-gen (or did you men something else?)

@champak
Copy link
Author

champak commented Dec 13, 2020

Thanks @DirectXMan12 I had been looking for a way to build a local version and use it with kubebuilder. I had built it like you suggested and then put in my path to prevent the kubebuilder Makefile from pulling the published version down. That worked OK but had wondered if there is better way etc.

@adux6991
Copy link

I had the same issue!!! It had been extremely puzzling until I realized there is something to do with controller-gen. Thank you for raising it and developers for fixing it!

@dvaldivia
Copy link
Contributor

@champak did you found a solution?

@dvaldivia
Copy link
Contributor

dvaldivia commented Jan 26, 2021

It's still not clear to me what is causing the embedded ObjectMeta to be lost, if the original CRD "v1" had it, wouldn't it be transmitted to the webhook and then the conversion code would move it over?

@fejta-bot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 16, 2021
@ns-jisorce
Copy link

Fixed by #557 no ?

@k8s-triage-robot
Copy link

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-ci-robot 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 Aug 4, 2021
@k8s-triage-robot
Copy link

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
Copy link
Contributor

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

@sfc-gh-llin
Copy link

Yes, it's fixed now. Use the controller-gen crd option crd:generateEmbeddedObjectMeta=true will work

@imaskm
Copy link

imaskm commented Sep 29, 2022

Hi guys, I tried with "crd:generateEmbeddedObjectMeta=true" and generated manifests and go code again but it still isn't working

my makefile targets:
`
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
$(CONTROLLER_GEN) crd:generateEmbeddedObjectMeta=true rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases

generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
$(CONTROLLER_GEN) crd:generateEmbeddedObjectMeta=true object:headerFile="hack/boilerplate.go.txt" paths="./..."

`

@imaskm
Copy link

imaskm commented Sep 29, 2022

Found the issue, I was using Makefile generated by Kubebuilder, after adding this flag "crd:generateEmbeddedObjectMeta=true" to "manifests" target, somehow the location of generated crd yaml was changed and I kept applying the older ones.

@Algatux
Copy link

Algatux commented Apr 14, 2023

Using the "crd:generateEmbeddedObjectMeta=true" flag fixes the issue but it is generating two versions of the manifests:

  • one fixed but in the wrong directory (config root)
  • one still broken inside the crd base directory

michi-covalent added a commit to cilium/tetragon that referenced this issue Sep 19, 2023
- Add WorkloadType and WorkloadObject fields to PodInfo.
- Export workload.GetWorkloadMetaFromPod() function so that the operator
  can call it to set WorkloadType and WorkloadObject fields.
- Update equal() function to take these fields into account.

I opted for defining a new WorkloadObjectMeta type instead of using
metav1.ObjectMeta to avoid generating unnecessary "unknown field" log
messages. See [1] for additional context.

[1]: kubernetes-sigs/controller-tools#448

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
michi-covalent added a commit to cilium/tetragon that referenced this issue Sep 20, 2023
- Add WorkloadType and WorkloadObject fields to PodInfo.
- Export workload.GetWorkloadMetaFromPod() function so that the operator
  can call it to set WorkloadType and WorkloadObject fields.
- Update equal() function to take these fields into account.

I opted for defining a new WorkloadObjectMeta type instead of using
metav1.ObjectMeta to avoid generating unnecessary "unknown field" log
messages. See [1] for additional context.

[1]: kubernetes-sigs/controller-tools#448

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@oscartrullols
Copy link

Using the "crd:generateEmbeddedObjectMeta=true" flag fixes the issue but it is generating two versions of the manifests:

  • one fixed but in the wrong directory (config root)
  • one still broken inside the crd base directory

I was also getting similar results until I understood how the command works. At first, I was wrongly adding the crd:generateEmbeddedObjectMeta=true to the command and that adds a new generator outputing its own results. what we really want here is to replace the already present crd generator with the crd:generateEmbeddedObjectMeta=true.

That solved my issues without generating any extra files at unexpected folders

michi-covalent added a commit to cilium/tetragon that referenced this issue Sep 20, 2023
- Add WorkloadType and WorkloadObject fields to PodInfo.
- Export workload.GetWorkloadMetaFromPod() function so that the operator
  can call it to set WorkloadType and WorkloadObject fields.
- Update equal() function to take these fields into account.

I opted for defining a new WorkloadObjectMeta type instead of using
metav1.ObjectMeta to avoid generating unnecessary "unknown field" log
messages. See [1] for additional context.

[1]: kubernetes-sigs/controller-tools#448

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@Antirecord
Copy link

Antirecord commented Oct 16, 2023

Using the "crd:generateEmbeddedObjectMeta=true" flag fixes the issue but it is generating two versions of the manifests:

  • one fixed but in the wrong directory (config root)
  • one still broken inside the crd base directory

I was also getting similar results until I understood how the command works. At first, I was wrongly adding the crd:generateEmbeddedObjectMeta=true to the command and that adds a new generator outputing its own results. what we really want here is to replace the already present crd generator with the crd:generateEmbeddedObjectMeta=true.

That solved my issues without generating any extra files at unexpected folders

Hi! I fixed my makefile, but 'controller-gen' generates the role.yaml in crd/bases/ . What i should do ?

.PHONY: manifests manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. $(CONTROLLER_GEN) rbac:roleName=manager-role crd:generateEmbeddedObjectMeta=true webhook paths="./..." output:artifacts:config=config/crd/bases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
Development

No branches or pull requests