Remove Elasticsearch field from CRDs #1687
Conversation
Use an annotation to manage association configuration instead of the publicly visible Elasticsearch field in Kibana and APM objects.
charith-elastic
added
>enhancement
There was a problem hiding this comment.
This looks very clean and there are some nice improvements wrt repeated code in here. I am 👍 on hiding the association details from the user as we know this causes confusion. I am a bit unsure about the how. I have some concerns about the approach proposed in this PR, lumping everything into a JSON object, in particular, but am willing to be convinced that this is OK
| // Fetch the ApmServer resource | ||
| as := &apmv1alpha1.ApmServer{} | ||
| err := r.Get(request.NamespacedName, as) | ||
| defer common.LogReconciliationRun(log, request, &r.iteration)() |
There was a problem hiding this comment.
Big 👍 but I would probably have done it in a separate PR
There was a problem hiding this comment.
Since I had to modify the reconciliation loops of almost all the operators anyway, I decided to clean them up a little bit and in the process included this tiny change which has bugged me for quite a while. In hindsight, I probably should have done it in a different PR as you say :)
|
|
||
| if err := json.Unmarshal(unsafeStringToBytes(serializedConf), &assocConf); err != nil { | ||
| return nil, errors.Wrapf(err, "failed to extract association configuration") | ||
| } |
There was a problem hiding this comment.
I feel a bit ambiguous about this serialisation format. I know kubectl does the same thing. But somehow individual annotations seem better to me, but open for discussion WDYT?
There was a problem hiding this comment.
I started by having separate annotations for each field but the implementation started to get a bit too repetitive. Generally, all of the fields contained here are accessed during a reconciliation run (more than once for some fields) so in terms of the lines of code involved to extract them and the associated performance implications, individual annotations per field didn't seem like an attractive option to me.
- There's no case for "lazy" access so might as well serialize/deserialize a single large blob instead of multiple smaller blobs.
- Increased cognitive load of having to consider how each field should be stored and retrieved (should
AuthSecretNameandAuthSecretKeybe stored under a single annotation or two, what is the format etc.) - Right now, all values are primitive but in the future we may want to include more complex ones (Eg. multiple hosts for APM output) -- which would require some form of serialization anyway.
- Having all the related values stored under a single annotation signals their relationship more strongly than having separate annotations.
- JSON encoded annotation values are not unusual in the K8S world (kubectl, some ingresses, etc.) 😄
There was a problem hiding this comment.
👍 except for
Right now, all values are primitive but in the future we may want to include more complex ones (Eg. multiple hosts for APM output) -- which would require some form of serialization anyway.
I hope we don't add more attributes to this.
| // unsafeBytesToString converts a byte array to string without making extra allocations. | ||
| func unsafeBytesToString(b []byte) string { | ||
| return *(*string)(unsafe.Pointer(&b)) | ||
| } |
There was a problem hiding this comment.
Do we need these optimisations (yet) ? I have some doubts but defer to people with more operational Go experience.
There was a problem hiding this comment.
Probably not. It's just force of habit to me. I don't think there's a downside to keeping them but happy to reconsider if you feel otherwise.
There was a problem hiding this comment.
I think I'm fine using this kind of optimization, my only concern is to have it in the association package, maybe stringsutil would be more appropriate ?
There was a problem hiding this comment.
I am not sure whether the unsafe functions deserve to be publicly visible utility functions in stringsutil because they require careful consideration before use. My inclination is to keep them localised here for the time being and if we find ourselves needing them elsewhere, only then consider moving them to a utils package. WDYT?
There was a problem hiding this comment.
I think I agree with you. Would be nice to add a note in the comment about it.
| // unsafeBytesToString converts a byte array to string without making extra allocations. | ||
| func unsafeBytesToString(b []byte) string { | ||
| return *(*string)(unsafe.Pointer(&b)) | ||
| } |
There was a problem hiding this comment.
I think I'm fine using this kind of optimization, my only concern is to have it in the association package, maybe stringsutil would be more appropriate ?
As discussed in #1141 and #1653, this change removes the
Elasticsearchfield from Kibana and APM objects. It was primarily used by the operator to store information about the association but since it was a public field, it appeared in the CRD documentation and gave users the wrong impression that it was configurable.The operator now manages association information in the
association.k8s.elastic.co/es-confannotation instead.Fixes #1653