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

[Enhancement]: Replace x-kubernetes-preserve-unknown-fields in CRD for Kubernetes Resources object. #9851

Closed
alexandrst88 opened this issue Mar 19, 2024 · 10 comments · Fixed by #9857

Comments

@alexandrst88
Copy link

Related problem

There is still an issue when provision "Kafka" resource with Terraform Kubernetes Provider. There is not possible change resources without re-creation of the resource which leads Operator to delete and create cluster. Could you please to add it, or point me the desired design for CRD generator code and i will implement it by myself.

Suggested solution

#9508 but for kubernetes resources as well in 0.40.0, it was partially implemented in #9292 but it was closed.

Alternatives

No response

Additional context

No response

@scholzj
Copy link
Member

scholzj commented Mar 19, 2024

I think you have to be more specific about what is missing. As far as I know, we did all we could, and even that caused a lot of users some problems. So not sure what more are you asking for.

Could you please to add it, or point me the desired design for CRD generator code and i will implement it by myself.

The source codes are in this repository. Unsurprisingly, the CRD generator is in the crd-generator directory.

@msillence
Copy link

msillence commented Mar 21, 2024

I've not tested the new CRDs with terraform yet but we are using the kubectl provider https://github.com/gavinbunney/terraform-provider-kubectl with resource kubectl_manifest in our terraform to avoid this problem. You also need to include the provider in the module folder if you are using modules.
Of note the CRDs aren't upgraded by helm when you upgrade the operator you need to do that by hand. Thought I'd mention it in case you still had the old CRDs.

@alexandrst88
Copy link
Author

Should be fixed in #9857

@alexandrst88
Copy link
Author

I've not tested the new CRDs with terraform yet but we are using the kubectl provider https://github.com/gavinbunney/terraform-provider-kubectl with resource kubectl_manifest in our terraform to avoid this problem. You also need to include the provider in the module folder if you are using modules. Of note the CRDs aren't upgraded by helm when you upgrade the operator you need to do that by hand. Thought I'd mention it in case you still had the old CRDs.

I'm using official terraform kubernetes provider - https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs/resources/manifest. I checked CRD's from 0.40.0 and their still have x-kubernetes-preserve-unknown-field for resourcers fields.

@scholzj
Copy link
Member

scholzj commented Mar 21, 2024

I think doing it for the quantities is fine. But it is important to keep in mind that x-kubernetes-preserve-unknown-field is valid and does not require any deletion of a custom resource. So ultimately, this is a Terraform issue and one day you might not be able to work your way around it.

@alexandrst88
Copy link
Author

I think doing it for the quantities is fine. But it is important to keep in mind that x-kubernetes-preserve-unknown-field is valid and does not require any deletion of a custom resource. So ultimately, this is a Terraform issue and one day you might not be able to work your way around it.

Yes, but resources and limits are changing very often, #9857 also adding validation for values. The fabric8 lib that is using is in this project for crds, reflects correct type of the resources, but crd-generator make it as x-kubernetes-preserve-unknown-field. From my experience this field should defined type. like here https://github.com/fabric8io/kubernetes-client/blob/464e32eb7c8c05acd3ecd2fa90c0a295d6e80a59/java-generator/core/src/test/resources/spark-crd.yml#L901.

@scholzj
Copy link
Member

scholzj commented Mar 21, 2024

Yeah, as I said, I think this change is fine.

@ppatierno
Copy link
Member

Going to remove the "needs-triage" label because of the agreement and the PR already under review.

@scholzj
Copy link
Member

scholzj commented Mar 22, 2024

@ppatierno Maybe you should review the PR instead and maybe we can close this :-)

@ppatierno
Copy link
Member

@scholzj I haven't closed the issue. Of course it will be closed when PR is merged. I just removed the needs-triage label because I don't think it needs triage anymore taking into account there is a PR for it and we were in agreement here that it would have been good to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants