Empty Probes in Spec do not play nice with the go-client validation

[HoustonPutman]

Description

The Probes and all the fields within the probes (livenessProbe, readinessProbe, and all probe options), all have the //+optional tag added to make them optional in the CRD. However, when using the go client (from my experience writing unit tests for the solr-operator), I am still not allowed to submit a ZookeeperCluster with empty probes, or probes fields.

Importance

I can use the zkCluster.WithDefaults() method before submitting the object to the kube-client, however that really ties the Solr Operator down to making sure the running version of the Zookeeper Operator matches the version that the Solr Operator has in its dependencies. It's a little safer if the Solr Operator can just make changes to the fields it cares about, and let the Zookeeper Operator do the defaulting.

Location

https://github.com/pravega/zookeeper-operator/blob/master/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go#L154
https://github.com/pravega/zookeeper-operator/blob/master/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go#L157

Suggestions for an improvement

I think this can be fixed by merely adding ,omitempty on the json part of the field variables.

For the probe options, it might be better to make the minimums 1, add the ,omitempty and keep the // +optional tag. That way it only prints out the values that have been provided. This is unrelated to the error I'm seeing, just a small improvement I think.

[anishakj]

@HoustonPutman I did not correctly get the issue here. Probe fields are not mandatory in the CRD. Adding +optional is equivalent to omitempty

[HoustonPutman]

So I believe the omitempty is used for the json serialization of the object in the client, and the optional just makes it not necessary in the CRD. So when omitempty is not specified, the client tries to send a null value to the API Server, which gets rejected. Not sure if the validation is client-side or server-side.

Either way I was confused as well, it should be ok. But it definitely didn't work when trying to integrate the new version of the CRD/API with the Solr Operator unit tests.

Should be an easy fix however.

[anishakj]

So I believe the omitempty is used for the json serialization of the object in the client, and the optional just makes it not necessary in the CRD. So when omitempty is not specified, the client tries to send a null value to the API Server, which gets rejected. Not sure if the validation is client-side or server-side.

Either way I was confused as well, it should be ok. But it definitely didn't work when trying to integrate the new version of the CRD/API with the Solr Operator unit tests.

Should be an easy fix however.

@HoustonPutman Are you getting validation errors while installing zookeeper with probes not specified. Could you please send the error message you are getting? Is the errors are coming from openapi validation

[HoustonPutman]

I'm getting this error when using the go client to create a ZookeeperCluster:

ZookeeperCluster.zookeeper.pravega.io "foo-clo-solrcloud-zookeeper" is invalid: spec.probes: Invalid value: "null": spec.probes in body must be of type object: "null"

The error object that is returned:

[anishakj]

@HoustonPutman If we install zookeeper cluster using charts, without specifying probes also, it is working for us. Would you like to raise a PR if providing omitempty solve the issue you are facing?