strimzi / strimzi-kafka-operator

Apache Kafka® running on Kubernetes
https://strimzi.io/
Apache License 2.0
4.87k stars 1.3k forks source link

[Bug] "prioroty" field in kafkatopics.kafka.strimzi.io CRD is not recognized by the API server #1909

Closed yuha0 closed 5 years ago

yuha0 commented 5 years ago

Describe the bug kafkatopics.kafka.strimzi.io has a priority field in CustomResourceColumnDefinition: https://github.com/strimzi/strimzi-kafka-operator/blob/master/install/topic-operator/04-Crd-kafkatopic.yaml#L30 However, this field is not returned in a GET call by the API server:

$ kubectl get CustomResourceDefinition -oyaml kafkatopics.kafka.strimzi.io | grep JSONPath: -B2 -A 5
spec:
  additionalPrinterColumns:
  - JSONPath: .spec.partitions
    description: The desired number of partitions in the topic
    name: Partitions
    type: integer
  - JSONPath: .spec.replicas
    description: The desired number of replicas of each partition
    name: Replication factor
    type: integer
  conversion:
    strategy: None

This means, if I execute kubectl apply multiple times, it will always show as configured instead of unchanged even if there's no change at all:

$ kubectl apply -f 04-Crd-kafkatopic.yaml
customresourcedefinition.apiextensions.k8s.io/kafkatopics.kafka.strimzi.io configured
$ kubectl apply -f 04-Crd-kafkatopic.yaml
customresourcedefinition.apiextensions.k8s.io/kafkatopics.kafka.strimzi.io configured
$ kubectl apply -f 04-Crd-kafkatopic.yaml
customresourcedefinition.apiextensions.k8s.io/kafkatopics.kafka.strimzi.io configured
...

To Reproduce Steps to reproduce the behavior:

  1. run kubectl apply -f https://raw.githubusercontent.com/strimzi/strimzi-kafka-operator/master/install/topic-operator/04-Crd-kafkatopic.yaml
  2. run kubectl apply -f https://raw.githubusercontent.com/strimzi/strimzi-kafka-operator/master/install/topic-operator/04-Crd-kafkatopic.yaml
  3. step 2 will return "configured" instead of "unchanged" because of the priority field

Expected behavior I am not sure if this is something we can fix in the CRD, or an issue in Kubernetes API server, but I would want the API server to recognize everything we defined in the CRD definition, including the priority field.

Additional context This is annoying if you have a GitOps pipeline to control strimzi CRDs -- our GitOps controller thinks the CRD is out of sync because the yaml file has priority field but the returned object from kubectl get does not have it

yuha0 commented 5 years ago

According to https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go#L236

priority is labeled with omitempty. This means it will not be returned if the value equals to 0 (0 means empty for an integer type).

Since 0 is already the default priority, I think it can be removed from the CRD definition.

scholzj commented 5 years ago

CC @tombentley

tombentley commented 5 years ago

TBH when I was implementing this I was just following the documentation here, which doesn't describe what the defaults for any of those fields are. I guess you're right at it could be omitted when 0, but it's harmless to include 0, so I don't see it as a priority to remove it.

tombentley commented 5 years ago

@yuha0 sorry, I didn't see all the context provided above. I guess it is problematic for you. It should be trivial to fix, actually. On line 246 or the CrdGenerator you can just add the priority property conditionally. Would you be able to provide a PR?

yuha0 commented 5 years ago

@tombentley Yeah, the issue is trivial. Once I understood how priority works I just changed the CRD in my cluster and it no longer bothers me.

Looks like this is just a small change in the CrdGenerator. I can make a PR when I get a chance. Do you think if we can remove all the priority: 0 lines in all the yaml files under https://github.com/strimzi/strimzi-kafka-operator/tree/master/install as well? I am using the topic operator in standalone mode in a cluster with GitOps, so the CRDs are version controlled and I have to modify the yaml files to make the GitOps pipeline happy.

tombentley commented 5 years ago

@yuha0 all the CRD YAMLs are generated by the CrdGenerator, so we need to make the fix there.