x-kubernetes-preserve-unknown-fields in CRD causes Terraform to delete and recreate manifest

[warwick-mitchell1]

Please use this to only for bug reports. For questions or when you need help, you can use the GitHub Discussions, our #strimzi Slack channel or out user mailing list.

Describe the bug
I deploy the kafka strimzi manifest using the Terraform kubernetes_manifest resource. It uses the CRD schema to determine if a resource can be updated or has to be recreated.

When objects specify the x-kubernetes-preserve-unknown-fields option in the CRD schema the Kubernetes Terraform provider can't tell if it's safe to update the fields so it instead deletes and recreates the manifest in Kubernetes. See this note for more background: hashicorp/terraform-provider-kubernetes#1928 (comment)

I recently tried to update the labels we use for the Kafka pods deployed by Strimzi. But by doing this it's causing Terraform to delete and recreate the Kafka cluster which isn't ideal and causes an outage.

I looked at the Strimzi CRD for Kafka cluster and for annotations and labels it uses the x-kubernetes-preserve-unknown-fields

  properties:
    annotations:
      description: Annotations added to the resource template.
        Can be applied to different resources such as `StatefulSets`,
        `Deployments`, `Pods`, and `Services`.
      type: object
      x-kubernetes-preserve-unknown-fields: true
    labels:
      description: Labels added to the resource template.
        Can be applied to different resources such as `StatefulSets`,
        `Deployments`, `Pods`, and `Services`.
      type: object
      x-kubernetes-preserve-unknown-fields: true

I have looked at other CRDs that seem to handle this ok like RabbitMQ and they use another option instead.

properties:
  annotations:
    additionalProperties:
      type: string
    type: object
  labels:
    additionalProperties:
      type: string
    type: object

I believe the correct way to specify the unknown keys under the labels and annotations objects is to use this addtionalProperties key.
https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

The same should apply to other unknown key values such as config options (see cruieseControl config in the terraform snippet below)

To Reproduce
Steps to reproduce the behavior:

1. Deploy Kafka Strimzi resource using terraform and the kubernetes_manifest terraform resource type
2. Once deployed add new labels or annotations and rerun terraform plan/apply
3. Take note of Kafka resource being deleted and recreated by Terraform

Expected behavior

I would expect to see Terraform simply update the Kuberntes manifest rather than deleting and recreating.

Environment (please complete the following information):

• Strimzi version: 0.32.0
• Installation method: Helm Chart
• Kubernetes cluster: GKE 1.23.14
• Infrastructure: GKE, Terraform

YAML files and logs

Terraform examples files: https://gist.github.com/warwick-mitchell1/3942059e431fbbc6f82ca97147d0e682

Terraform output

  # module.kafka_cluster.kubernetes_manifest.kafka_kafka_strimzi must be replaced
-/+ resource "kubernetes_manifest" "kafka_kafka_strimzi" {
      ~ manifest = {
          ~ metadata   = {
              + labels    = {
                  + app     = "kafka-cluster"
                  + kind    = "messaging"
                  + product = "kafka"
                  + team    = "platform"
                }
                name      = "kafka-strimzi"
                # (1 unchanged element hidden)
            }
          ~ spec       = {
              + cruiseControl  = {
                  + config = {
                      + "webserver.security.enable" = false
                      + "webserver.ssl.enable"      = false
                    }
                  + image  = "us.gcr.io/beamery-global/cruise-control:0.4.0"
                }
              ~ kafkaExporter  = {
                  ~ template   = {
                      ~ pod = {
                          ~ metadata        = {
                              + labels      = {
                                  + app     = "kafka-exporter"
                                  + kind    = "messaging"
                                  + product = "kafka"
                                  + team    = "platform"
                                }
                                # (1 unchanged element hidden)
                            }
                            # (1 unchanged element hidden)
                        }
                    }
                    # (2 unchanged elements hidden)
                }
                # (3 unchanged elements hidden)
            }
            # (2 unchanged elements hidden)

Additional context

[scholzj]

I do not see anything that would be invalid in our CRD definition. It is an object without a defined structure, so we preserve the unknown fields. And it certainly it does not say anything about needing to delete and recreate the resource. So I have no idea why Terraform behaves like that - that is probably something you would need to ask them.

Changing this just like that would be a high-risk change as these things tend to often fix something for one tool and break it for 5 other tools.

[warwick-mitchell1]

See the note from the person from Hashicorp I linked to in my OP

When x-kubernetes-preserve-unknown-fields attribute is present in a CRD, it signals that the CRD does not contain complete schema information for the object it describes. Most of the times this is due to the CRD authors not defining the schema in all detail to begin with. Without this information, the provider cannot guarantee that the attributes marked with this flag will not change their type as it get mutated by clients, which would be considered a fatal violation by Terraform. To prevent this, the provider disables updates to objects of such CRDs and instead it replaces it every time there is a change.

Labels, annotations and even config are a defined structure. It's just the property names aren't known. But by using the following you're specifying any additional properties will have a value of type string thus allowing something like terraform to validate the document.

    additionalProperties:
      type: string

[scholzj]

It is not a defined structure without known names of course. I'm not saying there is no other way to do it, but I do not see anything against the spec in how we do it. And there is nothing in the spec that says that without schema you cannot update the resource.

As I said, I'm sorry, but we can not run wildly around and modify the CRDs every time there is some tool that handles it only this way because there might be other tools that get broken by it. This needs a more solid explanation why this is invalid then just it does not fit Terraform way of working.

TBH, especially the idea that if someone uses x-kubernetes-preserve-unknown-fields they just delete and create the resource is super dangerous. Nobody might remember this discussion in a few months and might change it back to x-kubernetes-preserve-unknown-fields just because another tool comes saying it should be like that. And then what? Does it suddenly delete your Kafka cluster in production because of it? That seems insane.

[warwick-mitchell1]

Totally agreed it's a bit over the top by Terraform. But it is on me to check the plan before hitting that apply button so at least I can prevent it happening.

I understand your concerns about other tooling and flip flopping of the fields in the future. But this is the same way they define labels and annotations in the Kubernetes openapi specs.

kubectl proxy --port=8080 &
curl localhost:8080/openapi/v2 | jq '.definitions."io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta".properties.labels'
{
  "description": "Map of string keys and values that can be used to organize and categorize (scope and select) objects. May match selectors of replication controllers and services. More info: http://kubernetes.io/docs/user-guide/labels",
  "type": "object",
  "additionalProperties": {
    "type": "string"
  }
}

I would hope that's the ideal standard to replicate.

[scholzj]

Ok, that is a bit more convincing argument I guess. We would still need to think about backward compatibility. But I guess someone might look into it. We would also need to see how it fits in the code as there are other places where we expect more complex objects. I opened #7985 to get it triaged on the next community call.

[erezblm]

I'm just experiencing the same issue and every time i change something in the configs it destroys my resources including topic storage
@warwick-mitchell1 have you find a solution for the mid-time how to make terraform not destroying the resources on every update?

[msillence]

I'm using gavinbunney/kubectl provider to deploy the CRD, note if you're using this under a module to also have to define (but not configure) it in the module

[warwick-mitchell1]

Yeah we switched to the kubectl provider as well

[erezblm]

Oh interesting! good to hear, i'll check it out..
out of interest, have you moved using it for all your kubernetes resources, or just for solving the Strimzi issue?
probably will require to destroy everything in order to move to it

[warwick-mitchell1]

Moved to the kubectl provider only when there have been issues.
To move we manually removed from state and then reimported

[erezblm]

great, that's good to hear, thanks for the help! will do the same until there'll be a strimzi solution for that