fix(spire): preserve webhook order in patch hooks (#918)

* fix(spire-server): preserve webhook order in patch hooks

Signed-off-by: Christoph Manns <[email protected]>

* test(spire): cover webhook patch order

Signed-off-by: Christoph Manns <[email protected]>

* fix(spire-server): honor effective hook setting

Signed-off-by: Christoph Manns <[email protected]>

* Apply suggestion from @kfox1111

Signed-off-by: kfox1111 <[email protected]>

---------

Signed-off-by: Christoph Manns <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Co-authored-by: kfox1111 <[email protected]>
Co-authored-by: kfox1111 <[email protected]>

charts/spire/charts/spire-server/templates/controller-manager-webhook.yaml

@@ -1,3 +1,4 @@
+{{- $installAndUpgradeHooksEnabled := dig "installAndUpgradeHooks" "enabled" .Values.controllerManager.installAndUpgradeHook.enabled .Values.global }}
 {{- if not .Values.externalServer }}
 {{- if eq .Values.controllerManager.staticManifestMode "off" }}
 {{- if and (eq (.Values.controllerManager.enabled | toString) "true") .Values.controllerManager.validatingWebhookConfiguration.enabled }}
@@ -12,7 +13,7 @@ webhooks:
         name: {{ include "spire-controller-manager.fullname" . }}-webhook
         namespace: {{ include "spire-server.namespace" . }}
         path: /validate-spire-spiffe-io-v1alpha1-clusterfederatedtrustdomain
-    {{- if eq (.Values.controllerManager.installAndUpgradeHook.enabled | toString) "true" }}
+    {{- if eq ($installAndUpgradeHooksEnabled | toString) "true" }}
     failurePolicy: Ignore # Actual value to be set by post install/upgrade hooks
     {{- else }}
     failurePolicy: {{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}
@@ -30,7 +31,11 @@ webhooks:
         name: {{ include "spire-controller-manager.fullname" . }}-webhook
         namespace: {{ include "spire-server.namespace" . }}
         path: /validate-spire-spiffe-io-v1alpha1-clusterspiffeid
+    {{- if eq ($installAndUpgradeHooksEnabled | toString) "true" }}
     failurePolicy: Ignore # Actual value to be set by post install/upgrade hooks
+    {{- else }}
+    failurePolicy: {{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}
+    {{- end }}
     name: vclusterspiffeid.kb.io
     rules:
       - apiGroups: ["spire.spiffe.io"]

charts/spire/charts/spire-server/templates/post-install-hook.yaml

@@ -85,11 +85,11 @@ spec:
             {
               "webhooks":[
                 {
-                  "name":"vclusterspiffeid.kb.io",
+                  "name":"vclusterfederatedtrustdomain.kb.io",
                   "failurePolicy":"{{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}"
                 },
                 {
-                  "name":"vclusterfederatedtrustdomain.kb.io",
+                  "name":"vclusterspiffeid.kb.io",
                   "failurePolicy":"{{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}"
                 }
               ]

charts/spire/charts/spire-server/templates/post-upgrade-hook.yaml

@@ -85,11 +85,11 @@ spec:
             {
               "webhooks":[
                 {
-                  "name":"vclusterspiffeid.kb.io",
+                  "name":"vclusterfederatedtrustdomain.kb.io",
                   "failurePolicy":"{{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}"
                 },
                 {
-                  "name":"vclusterfederatedtrustdomain.kb.io",
+                  "name":"vclusterspiffeid.kb.io",
                   "failurePolicy":"{{ .Values.controllerManager.validatingWebhookConfiguration.failurePolicy }}"
                 }
               ]

charts/spire/charts/spire-server/templates/pre-upgrade-hook.yaml

@@ -85,11 +85,11 @@ spec:
             {
               "webhooks":[
                 {
-                  "name":"vclusterspiffeid.kb.io",
+                  "name":"vclusterfederatedtrustdomain.kb.io",
                   "failurePolicy":"Ignore"
                 },
                 {
-                  "name":"vclusterfederatedtrustdomain.kb.io",
+                  "name":"vclusterspiffeid.kb.io",
                   "failurePolicy":"Ignore"
                 }
               ]

tests/unit/spire_test.go

@@ -1,6 +1,10 @@
 package unit_test
 
 import (
+	"encoding/json"
+	"io"
+	"strings"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -8,8 +12,71 @@ import (
 	helmloader "helm.sh/helm/v3/pkg/chart/loader"
 	helmutil "helm.sh/helm/v3/pkg/chartutil"
 	helmengine "helm.sh/helm/v3/pkg/engine"
+	yamlutil "k8s.io/apimachinery/pkg/util/yaml"
 )
 
+type renderedWebhook struct {
+	Name          string `json:"name"`
+	FailurePolicy string `json:"failurePolicy"`
+}
+
+type renderedDocument struct {
+	Kind     string `json:"kind"`
+	Metadata struct {
+		Annotations map[string]string `json:"annotations"`
+	} `json:"metadata"`
+	Spec struct {
+		Template struct {
+			Spec struct {
+				Containers []struct {
+					Args []string `json:"args"`
+				} `json:"containers"`
+			} `json:"spec"`
+		} `json:"template"`
+	} `json:"spec"`
+	Webhooks []renderedWebhook `json:"webhooks"`
+}
+
+func decodeRenderedDocuments(rendered string) ([]renderedDocument, error) {
+	decoder := yamlutil.NewYAMLOrJSONDecoder(strings.NewReader(rendered), 4096)
+	var documents []renderedDocument
+	for {
+		var document renderedDocument
+		err := decoder.Decode(&document)
+		if err == io.EOF {
+			return documents, nil
+		}
+		if err != nil {
+			return nil, err
+		}
+		if document.Kind != "" {
+			documents = append(documents, document)
+		}
+	}
+}
+
+func patchWebhookNames(job renderedDocument) ([]string, error) {
+	for _, container := range job.Spec.Template.Spec.Containers {
+		for index, arg := range container.Args {
+			if arg != "-p" || index+1 >= len(container.Args) {
+				continue
+			}
+			var patch struct {
+				Webhooks []renderedWebhook `json:"webhooks"`
+			}
+			if err := json.Unmarshal([]byte(container.Args[index+1]), &patch); err != nil {
+				return nil, err
+			}
+			names := make([]string, 0, len(patch.Webhooks))
+			for _, webhook := range patch.Webhooks {
+				names = append(names, webhook.Name)
+			}
+			return names, nil
+		}
+	}
+	return nil, nil
+}
+
 func ValueStringRender(chart *helmchart.Chart, values string) (map[string]string, error) {
 	v, err := helmutil.ReadValues([]byte(values))
 	if err != nil {
@@ -663,6 +730,86 @@ spire-server:
 			Expect(err).Should(Succeed())
 		})
 	})
+	Describe("spire-server webhook patch order", func() {
+		It("preserves the rendered webhook order in every strategic-merge hook", func() {
+			objs, err := ValueStringRender(chart, `
+global:
+  installAndUpgradeHooks:
+    enabled: true
+spire-server:
+  enabled: true
+  controllerManager:
+    enabled: true
+`)
+			Expect(err).Should(Succeed())
+
+			canonicalDocuments, err := decodeRenderedDocuments(objs["spire/charts/spire-server/templates/controller-manager-webhook.yaml"])
+			Expect(err).Should(Succeed())
+			Expect(canonicalDocuments).Should(HaveLen(1))
+			Expect(canonicalDocuments[0].Kind).Should(Equal("ValidatingWebhookConfiguration"))
+			canonicalNames := make([]string, 0, len(canonicalDocuments[0].Webhooks))
+			for _, webhook := range canonicalDocuments[0].Webhooks {
+				canonicalNames = append(canonicalNames, webhook.Name)
+			}
+
+			for _, hook := range []struct {
+				name     string
+				template string
+			}{
+				{name: "post-install", template: "spire/charts/spire-server/templates/post-install-hook.yaml"},
+				{name: "pre-upgrade", template: "spire/charts/spire-server/templates/pre-upgrade-hook.yaml"},
+				{name: "post-upgrade", template: "spire/charts/spire-server/templates/post-upgrade-hook.yaml"},
+			} {
+				documents, err := decodeRenderedDocuments(objs[hook.template])
+				Expect(err).Should(Succeed())
+				var jobs []renderedDocument
+				for _, document := range documents {
+					if document.Kind == "Job" && document.Metadata.Annotations["helm.sh/hook"] == hook.name {
+						jobs = append(jobs, document)
+					}
+				}
+				Expect(jobs).Should(HaveLen(1), hook.name)
+				actualNames, err := patchWebhookNames(jobs[0])
+				Expect(err).Should(Succeed())
+				Expect(actualNames).Should(Equal(canonicalNames), hook.name)
+			}
+		})
+	})
+	Describe("spire-server webhook hooks disabled", func() {
+		It("uses the configured failure policy and omits lifecycle Jobs", func() {
+			objs, err := ValueStringRender(chart, `
+global:
+  installAndUpgradeHooks:
+    enabled: false
+spire-server:
+  controllerManager:
+    enabled: true
+    validatingWebhookConfiguration:
+      failurePolicy: Fail
+`)
+			Expect(err).Should(Succeed())
+
+			canonicalDocuments, err := decodeRenderedDocuments(objs["spire/charts/spire-server/templates/controller-manager-webhook.yaml"])
+			Expect(err).Should(Succeed())
+			Expect(canonicalDocuments).Should(HaveLen(1))
+			Expect(canonicalDocuments[0].Webhooks).Should(HaveLen(2))
+			for _, webhook := range canonicalDocuments[0].Webhooks {
+				Expect(webhook.FailurePolicy).Should(Equal("Fail"))
+			}
+
+			for _, template := range []string{
+				"spire/charts/spire-server/templates/post-install-hook.yaml",
+				"spire/charts/spire-server/templates/pre-upgrade-hook.yaml",
+				"spire/charts/spire-server/templates/post-upgrade-hook.yaml",
+			} {
+				documents, err := decodeRenderedDocuments(objs[template])
+				Expect(err).Should(Succeed())
+				for _, document := range documents {
+					Expect(document.Kind).ShouldNot(Equal("Job"), template)
+				}
+			}
+		})
+	})
 	Describe("spire-server.dataStore.sql.postgres passwordless", func() {
 		It("omits password and the -dbpw Secret for cert auth with an empty password", func() {
 			objs, err := ValueStringRender(chart, `