Fix Logstash templating issue in Helm chart

[naemono]

Fixes #8000

This change fixes a bug in the helm templating of Logstash and does not add .spec.pipelines when .spec.pipelinesRef is also defined. In addition this adds a test to ensure that the field pipelines is missing in spec when pipelinesRef is also defined.

[thbkrkr]

Imagine if the user sets both pipelines and pipelinesRef fields, then only pipelinesRef will be rendered without error. What do you think about adding a validation to prevent setting both fields?
Nit: also group pipeline related bits together.

diff --git a/deploy/eck-stack/charts/eck-logstash/templates/logstash.yaml b/deploy/eck-stack/charts/eck-logstash/templates/logstash.yaml
index 3c4deb1bc..162ec317a 100644
--- a/deploy/eck-stack/charts/eck-logstash/templates/logstash.yaml
+++ b/deploy/eck-stack/charts/eck-logstash/templates/logstash.yaml
@@ -31,10 +31,6 @@ spec:
   configRef:
     {{- toYaml . | nindent 4 }}
   {{- end }}
-  {{- with .Values.pipelinesRef }}
-  pipelinesRef:
-    {{- toYaml . | nindent 4 }}
-  {{- end }}
   {{- with .Values.podTemplate }}
   podTemplate:
     {{- toYaml . | nindent 4 }}
@@ -43,8 +39,14 @@ spec:
   monitoring:
     {{- toYaml . | nindent 4 }}
   {{- end }}
-
-  {{- if not (.Values.pipelinesRef)  }}
+  {{- if and .Values.pipelines .Values.pipelinesRef }}
+  {{- fail "pipelines and pipelinesRef are mutually exclusive!" }}
+  {{- end }}
+  {{- with .Values.pipelinesRef }}
+  pipelinesRef:
+    {{- toYaml . | nindent 4 }}
+  {{- end }}
+  {{- if .Values.pipelines  }}
   pipelines: {{ toYaml .Values.pipelines | nindent 4 }}
   {{- end }}
   volumeClaimTemplates: {{ toYaml .Values.volumeClaimTemplates | nindent 4 }}
diff --git a/deploy/eck-stack/charts/eck-logstash/values.yaml b/deploy/eck-stack/charts/eck-logstash/values.yaml
index a2d1d289a..e68694852 100644
--- a/deploy/eck-stack/charts/eck-logstash/values.yaml
+++ b/deploy/eck-stack/charts/eck-logstash/values.yaml
@@ -78,11 +78,11 @@ monitoring: {}
   #     namespace: observability
 
 # The Logstash pipelines, the ECK equivalent to pipelines.yml
+# NOTE: The `pipelines` and `pipelinesRef` fields are mutually exclusive. Only one of them should be defined at a time, as using both may cause conflicts.
 # ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-logstash-configuration.html#k8s-logstash-pipelines
 #
 pipelines: []
-
-pipelinesRef: {}
+# pipelinesRef:
 #  secretRef:
 #    secretName: ''

[naemono]

Thanks @thbkrkr , this makes a lot of sense. Upon further looking at the documentation in the values file, I'm noticing some other issues I'm going to double check/fix:

This doesn't appear to be correct, according to the documentation:

-pipelinesRef: {}
-#  secretRef:
-#    secretName: ''

It should be:

# pipelinesRef:
#   secretName: ''

Also, do we have the same situation for config and configRef? I believe we do. I'm going to double check that as well, as those seem to be mutually exclusive also.

[thbkrkr]

Well seen, yes pipelinesRef. secretRef is not valid

and config / configRef are also mutually exclusive.

cloud-on-k8s/pkg/controller/logstash/validation/validations.go

Line 75 in e415ac0

msg := "Specify at most one of [`config`, `configRef`], not both"

[naemono]

Ok @thbkrkr , the values file has been updated, the template will now fail on this as well, and there are 2x tests to ensure this functionality. Ready for next set of 👀 when you can. Thanks!

[thbkrkr]

Good improvements! ✨