Breaking change caused by new ingress-nginx custom header annotation

[cterence]

#106 changed the way to add custom headers, but it was not correctly implemented as my ingress-nginx was responding 503 errors after the chart update.

The change is broken in two ways:

• No namespace prefix before the configmap name in nginx.ingress.kubernetes.io/custom-headers: {{ $ingress.customHeadersConfigMap }} : ingress-nginx therefore tries to find the configmap in its own namespace, which is not where vaultwarden is deployed in my cluster
  • Adding customHeadersConfigMap: "vaultwarden/custom-headers-configmap" in the helm values fixes the issue (cterence/homelab-gitops@0e5844e)
• Applying the previous fix caused an error in ingress-nginx: error reading Ingress annotation" err="location denied, reason: header Request-Id is not allowed, defined allowed headers inside global-allowed-response-headers []".
  • Reading https://github.com/kubernetes/ingress-nginx/blob/main/docs/user-guide/nginx-configuration/annotations.md#custom-headers, a user needs to add the response header to his ingress-nginx configmap before it can be used in the custom-header annotation
  • I was able to fix the issue by complying with the instructions above cterence/homelab-gitops@51ab22f

The first problem is a quick fix in the helm templates (nginx.ingress.kubernetes.io/custom-headers: {{ .Release.Namespace }}/{{ $ingress.customHeadersConfigMap }}) but the second is not trivial as it's a breaking change which requires users of this chart to modify their ingress-nginx configuration to support this new feature.

I would recommend reverting this change and releasing a new major version for this chart if the intention is to use this new method for passing custom headers.

[guerzon]

Thanks for submitting the issue @cterence. Perhaps this is also the issue that @RealKelsar encounters.

I actually found the note on global-allowed-response-headers but I let it go after not being able to replicate an issue in my lab environments. My bad!

Having said that, I prefer not to revert the change since it also served as risk avoidance on a CVE which could impact multi-tenant clusters. Instead, I would prefer to add a documentation/warning about the controller-level requirement and make this values configuration (and the corresponding ConfigMap optional).

[RealKelsar]

Yes, those two changes make it work for me again. Sorry, simply had no time yet to investigate.

[cterence]

Hello @guerzon,your proposed solution sounds good to me, thank you!

[RealKelsar]

Sorry, but that makes it worse.

Without configmap it does not work at all.

With the new format of the customHeadersConfigMap: the ingress gets the wrong Annotation, instead of the configMap Name it gets the content, which does not work.

The Default should be the configMap with the Req-ID and the default used for the Ingress should be

$vaultwardenNamespace/customHeadersConfigMap

I like the possibility to add more Headers easily thou.

[guerzon]

Sorry, but that makes it worse.

Without configmap it does not work at all.

With the new format of the customHeadersConfigMap: the ingress gets the wrong Annotation, instead of the configMap Name it gets the content, which does not work.

The Default should be the configMap with the Req-ID and the default used for the Ingress should be

$vaultwardenNamespace/customHeadersConfigMap

I like the possibility to add more Headers easily thou.

Hi @RealKelsar, are you able to test my change in #113? Hope this should be enough

[RealKelsar]

#113 seems to work and I learned how to pull a PR with Argo :)

Somewhere on my list is Helm chart writing...

[guerzon]

Thanks, I have merged #113.