Helm chart's SCC should have volumes in alphabetical order

kbreit-insight commented 4 months ago

Overview of the Issue

The list of volumes at https://github.com/hashicorp/consul-k8s/blob/1b04a36d64e09410b3aae4f83b135b9fa3bb98ec/charts/consul/templates/cni-securitycontextconstraints.yaml#L42 should be in alphabetical order. The problem this causes is I am deploying Consul via helm chart in ArgoCD and it won't fully synchronize because it's expecting the hostPath to be at the bottom. However, Kubernetes appears to sort them as here's the current resource section:

volumes:
- configMap
- downwardAPI
- emptyDir
- hostPath
- persistentVolumeClaim
- projected
- secret

Reproduction Steps

Deploy Consul on OpenShift with the CNI enabled. Optionally, deploy via Argo but that should only show it's being applied in a different order.

Expected behavior

I would expect the order to stay the same.

Environment details

OpenShift 4.15 (Kubernetes 1.28.x) Consul 1.19 Helm chart v1.5.1

zalimeni commented 3 months ago

Thanks for the report, @kbreit-insight !

Based on some initial poking into OpenShift source, I think what you're suggesting makes sense - though the silent forced sorting in OpenShift is (AFAICT) an undocumented behavior, and makes use cases like Argo's diffing unnecessarily brittle. It seems there's an Argo feature request to address cases like this, which could help in the future.

Side note: interestingly, the order consul-k8s is using is almost based on OpenShift behavior, but seems a bit off:

This code: https://github.com/openshift/kubernetes/blob/6892b57d65d25fb0588693bce3d338d8b8b1d2b4/openshift-kube-apiserver/admission/customresourcevalidation/securitycontextconstraints/defaults.go#L51 will by default add hostPath to the end of the array if it’s missing, and the allowHostDirVolumePlugin flag is set
- That, combined w/ the wrapping deserialize/serialize logic should normalize the data, forcing a sort on existing values.
However, if SetDefaults_SCC were called in isolation with allowHostDirVolumePlugin set and hostPath omitted - which seems possible here in the other use of the func in openshift-apiserver - it might be possible to see the unsorted order reflected in the consul-k8s template.
- Perhaps early testing of OpenShift support relied on the openshift-apiserver behavior rather than what we see in the openshift/kubernetes admission hooks. I'm trying to piece together history across both projects but haven't come up w/ a 100% clear explanation yet. @curtbushko may have some insight though it's been a while since he worked on this integration.

Regardless, I think your suggestion will work given both values are explicitly set in our template together. I'll probably include a comment in case future changes to the conditional might impact the expected order w/ inconsistent post-sort on the OpenShift side. I'll work on reproducing and testing the fix and comment back here when a PR is available.

In the meantime, I believe it's also possible to instruct Argo to ignore differences for specific fields, in case that's helpful as a stopgap.

zalimeni commented 3 months ago

Fix PR: https://github.com/hashicorp/consul-k8s/pull/4227

@kbreit-insight feel free to follow ^ for updates. Assuming it's merged in the next few weeks, it should go out with the next set of consul-k8s patch releases (currently expected ~end of August, though that date isn't yet set in stone). I'll close this issue once the PR is merged, but please comment back w/ any feedback if you run into trouble after testing.

hashicorp / consul-k8s