Fixed a bug in the nifi-cluster helm chart where the cluster.logbackConfig.replaceConfigMap couldn't be set

[r65535]

Q	A
Bug fix?	yes
New feature?	no
API breaks?	no
Deprecations?	no
Related tickets
License	Apache 2.0

What's in this PR?

The current cluster.logbackConfig logic in the nifi-cluster helm chart doesn't allow a user to override the logback.xml with a configMap.

Additional context

Currently if you set cluster.logbackConfig.replaceConfigMap and omit cluster.logbackConfig.replaceSecretConfig, the default secret value is added to the NiFiCluster, as well as the configmap value - setting both.

This change allows a user to set either replaceSecretConfig or replaceConfigMap and if unset, falls back to the default.

Checklist

• Implementation tested
• Error handling code meets the guideline
• Logging code meets the guideline
• User guide and development docs updated (if needed)
• Append changelog with changes

[juldrixx]

The two replaces (configmap vs. secret) are complementary, not exclusive. You can define the 2 and the result is a fusion of the 2.

https://github.com/konpyutaika/nifikop/blob/master/pkg/resources/nifi/secretconfig.go#L341-L361

The Helm should therefore make it possible to specialise in both independently. And in your change, it's either one or the other.

[r65535]

Thanks for the info, I didn't realise you could set both!

I think the helm-chart is still incorrectly applying a default logback in it's current configuration though. If I use the following helm values:

cluster:
  logbackConfig:
    replaceConfigMap:
      data: logback.xml
      name: cluster-config
      namespace: nifi
    replaceSecretConfig: {}

because cluster.logbackConfig.replaceSecretConfig is empty, the chart will apply the default template provided by the helm chart: https://github.com/konpyutaika/nifikop/blob/master/helm/nifi-cluster/templates/nifi-cluster.yaml#L91-L102, meaning it'll ignore the one I provided in my configmap?

[juldrixx]

Thanks for the info, I didn't realise you could set both!

I think the helm-chart is still incorrectly applying a default logback in it's current configuration though. If I use the following helm values:

cluster:
  logbackConfig:
    replaceConfigMap:
      data: logback.xml
      name: cluster-config
      namespace: nifi
    replaceSecretConfig: {}

because cluster.logbackConfig.replaceSecretConfig is empty, the chart will apply the default template provided by the helm chart: https://github.com/konpyutaika/nifikop/blob/master/helm/nifi-cluster/templates/nifi-cluster.yaml#L91-L102, meaning it'll ignore the one I provided in my configmap?

There is no "default" logback configuration.

    logbackConfig:
    {{- if .Values.cluster.logbackConfig.replaceSecretConfig }}
      replaceSecretConfig:
        data: {{ .Values.cluster.logbackConfig.replaceSecretConfig.data }}
        name: {{ .Values.cluster.logbackConfig.replaceSecretConfig.name }}
        namespace: {{ .Values.cluster.logbackConfig.replaceSecretConfig.namespace }}
    {{- else }}
      # the default
      replaceSecretConfig:
        data: logback.xml
        name: {{ include "nifi-cluster.fullname" . }}
        namespace: {{ .Release.Namespace }}
    {{- end }}
    {{- if .Values.cluster.logbackConfig.replaceConfigMap }}
      replaceConfigMap:
        data: {{ .Values.cluster.logbackConfig.replaceConfigMap.data }}
        name: {{ .Values.cluster.logbackConfig.replaceConfigMap.name }}
        namespace: {{ .Values.cluster.logbackConfig.replaceConfigMap.namespace }}
    {{- end }}

If you don't set replaceSecretConfig or replaceConfigMap. It will just fallback on the default Secret created here but there isn't any logback.xml in it. So NiFiKOp will use its default template present here without any addition.

[mh013370]

Thanks for the info, I didn't realise you could set both!
I think the helm-chart is still incorrectly applying a default logback in it's current configuration though. If I use the following helm values:

cluster:
  logbackConfig:
    replaceConfigMap:
      data: logback.xml
      name: cluster-config
      namespace: nifi
    replaceSecretConfig: {}

because cluster.logbackConfig.replaceSecretConfig is empty, the chart will apply the default template provided by the helm chart: https://github.com/konpyutaika/nifikop/blob/master/helm/nifi-cluster/templates/nifi-cluster.yaml#L91-L102, meaning it'll ignore the one I provided in my configmap?

There is no "default" logback configuration.

    logbackConfig:
    {{- if .Values.cluster.logbackConfig.replaceSecretConfig }}
      replaceSecretConfig:
        data: {{ .Values.cluster.logbackConfig.replaceSecretConfig.data }}
        name: {{ .Values.cluster.logbackConfig.replaceSecretConfig.name }}
        namespace: {{ .Values.cluster.logbackConfig.replaceSecretConfig.namespace }}
    {{- else }}
      # the default
      replaceSecretConfig:
        data: logback.xml
        name: {{ include "nifi-cluster.fullname" . }}
        namespace: {{ .Release.Namespace }}
    {{- end }}
    {{- if .Values.cluster.logbackConfig.replaceConfigMap }}
      replaceConfigMap:
        data: {{ .Values.cluster.logbackConfig.replaceConfigMap.data }}
        name: {{ .Values.cluster.logbackConfig.replaceConfigMap.name }}
        namespace: {{ .Values.cluster.logbackConfig.replaceConfigMap.namespace }}
    {{- end }}

If you don't set replaceSecretConfig or replaceConfigMap. It will just fallback on the default Secret created here but there isn't any logback.xml in it. So NiFiKOp will use its default template present here without any addition.

The nifi-cluster helm chart will use this configuration if there's not one provided as an override: https://github.com/konpyutaika/nifikop/blob/master/helm/nifi-cluster/config/logback.xml

But there's actually a bug here where it's not possible to override the logback.xml using a ConfigMap due to the logic here: https://github.com/konpyutaika/nifikop/blob/master/helm/nifi-cluster/templates/nifi-cluster.yaml#L105-L122

So that logic needs updating and perhaps the helm chart should not even provide a default and let the operator generate it from the template you've linked. But because there's a default in the helm chart, the operator's template won't be used as currently written.