Configs containing key: null result in invalid config

[david-kow]

Following up on #3041 (comment).

When Beat config is updated with a config that contains any key: null kubectls patching internals will interpret this as an indication to delete key from the dictionary. This is the expected behavior (https://tools.ietf.org/html/rfc7396).

The key: null is used often in processors configuration. Given that:

1. We have to avoid this in our defaults, samples and docs. At least for processors, using empty dictionary ({}) instead should suffice and shouldn't change Beat behavior.
2. For users applying a config containing key: null we need to provide a feedback that this is likely to cause not intended behavior. We could:
   1. validate processors path in the config specifically for empty entries
   2. use last-applied-config annotation to figure out whether the resource sent reflects user intention

[sebgl]

One alternative way to avoid this problem is to remove the config field entirely so there's no config interpreted by kubectl operations.
Instead we could only allow referencing configmaps/secrets that contain raw bytes (uninterpreted by k8s json/yaml processors):

apiVersion: beat.k8s.elastic.co/v1beta1
kind: Beat
metadata:
  name: filebeat-sample
spec:
  type: filebeat
  version: 7.7.0
  elasticsearchRef:
    name: elasticsearch-sample
  configRef:
    secretName: my-config
---
apiVersion: v1
kind: Secret
metadata:
  name: my-config
type: Opaque
stringData:
  beat.yaml: |-
    filebeat.autodiscover.providers:
    - node: ${NODE_NAME}
      type: kubernetes
      templates:
      - config:
        - paths: ["/var/log/containers/*${data.kubernetes.container.id}.log"]
          type: container
    processors:
    - add_cloud_metadata: null
    - add_host_metadata: null

The major downside is the lack of consistency with other CRDs: they all have the config field.

[david-kow]

I think I'd add configRef, but keep config around. For the most common case (processors) this is easy to catch through webhook/controller validation as null value for processor is just invalid configuration. We can expand it as we learn about other settings prone to this issue.

An additional thing we can do for all the other cases is to validate last-applied-config annotation against config field. As the annotation contains the exact resource that was applied, we can compare and see whether the resource and annotation match. This has the same issues that our unknown fields validation has (kubectl edit might not work unless annotation is removed or updated).

[david-kow]

After giving some more thought to possible validations I think we should only do a simple, targeted validation of processors field. Doing a generic validation seems problematic as we won't be able to distinguish the user intent between using key: null notation to really remove something (in which case annotation and the object won't match) and using that semantic accidentally.

Having a validation specifically for processors is fine, since we know that this list should not contain nulls.

[david-kow]

Given that we didn't have any users report issues around key: null so far, I'll be closing this issue without implementing an additional validation. It seems this is not common enough to justify a separate validation targetting a very specific path inside one of the config types.