🐛 Fix webhook cert duplicate volume mount path bug (#2002)

* Remove api-service certificate volume from operator deployment

Signed-off-by: Per Goncalves da Silva <[email protected]>

* Add tls.crt and tls.key constants

Signed-off-by: Per Goncalves da Silva <[email protected]>

* Ensure volumes with volume mounts referencing protected cert paths get replaced

Signed-off-by: Per Goncalves da Silva <[email protected]>

---------

Signed-off-by: Per Goncalves da Silva <[email protected]>
Co-authored-by: Per Goncalves da Silva <[email protected]>

Author: perdasilva

internal/operator-controller/rukpak/render/registryv1/generators/generators.go

@@ -26,15 +26,14 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
 )
 
-var certVolumeMounts = map[string]corev1.VolumeMount{
-	"apiservice-cert": {
-		Name:      "apiservice-cert",
-		MountPath: "/apiserver.local.config/certificates",
-	},
-	"webhook-cert": {
-		Name:      "webhook-cert",
-		MountPath: "/tmp/k8s-webhook-server/serving-certs",
-	},
+const (
+	tlsCrtPath = "tls.crt"
+	tlsKeyPath = "tls.key"
+)
+
+// volume mount name -> mount path
+var certVolumeMounts = map[string]string{
+	"webhook-cert": "/tmp/k8s-webhook-server/serving-certs",
 }
 
 // BundleCSVDeploymentGenerator generates all deployments defined in rv1's cluster service version (CSV). The generated
@@ -480,43 +479,35 @@ func getWebhookServicePort(wh v1alpha1.WebhookDescription) corev1.ServicePort {
 }
 
 func addCertVolumesToDeployment(dep *appsv1.Deployment, certSecretInfo render.CertSecretInfo) {
+	volumeMountsToReplace := sets.New(slices.Collect(maps.Keys(certVolumeMounts))...)
+	certVolumeMountPaths := sets.New(slices.Collect(maps.Values(certVolumeMounts))...)
+	for _, c := range dep.Spec.Template.Spec.Containers {
+		for _, containerVolumeMount := range c.VolumeMounts {
+			if certVolumeMountPaths.Has(containerVolumeMount.MountPath) {
+				volumeMountsToReplace.Insert(containerVolumeMount.Name)
+			}
+		}
+	}
+
 	// update pod volumes
 	dep.Spec.Template.Spec.Volumes = slices.Concat(
 		slices.DeleteFunc(dep.Spec.Template.Spec.Volumes, func(v corev1.Volume) bool {
-			_, ok := certVolumeMounts[v.Name]
-			return ok
+			return volumeMountsToReplace.Has(v.Name)
 		}),
 		[]corev1.Volume{
 			{
-				Name: "apiservice-cert",
-				VolumeSource: corev1.VolumeSource{
-					Secret: &corev1.SecretVolumeSource{
-						SecretName: certSecretInfo.SecretName,
-						Items: []corev1.KeyToPath{
-							{
-								Key:  certSecretInfo.CertificateKey,
-								Path: "apiserver.crt",
-							},
-							{
-								Key:  certSecretInfo.PrivateKeyKey,
-								Path: "apiserver.key",
-							},
-						},
-					},
-				},
-			}, {
 				Name: "webhook-cert",
 				VolumeSource: corev1.VolumeSource{
 					Secret: &corev1.SecretVolumeSource{
 						SecretName: certSecretInfo.SecretName,
 						Items: []corev1.KeyToPath{
 							{
 								Key:  certSecretInfo.CertificateKey,
-								Path: "tls.crt",
+								Path: tlsCrtPath,
 							},
 							{
 								Key:  certSecretInfo.PrivateKeyKey,
-								Path: "tls.key",
+								Path: tlsKeyPath,
 							},
 						},
 					},
@@ -529,15 +520,18 @@ func addCertVolumesToDeployment(dep *appsv1.Deployment, certSecretInfo render.Ce
 	for i := range dep.Spec.Template.Spec.Containers {
 		dep.Spec.Template.Spec.Containers[i].VolumeMounts = slices.Concat(
 			slices.DeleteFunc(dep.Spec.Template.Spec.Containers[i].VolumeMounts, func(v corev1.VolumeMount) bool {
-				_, ok := certVolumeMounts[v.Name]
-				return ok
+				return volumeMountsToReplace.Has(v.Name)
 			}),
-			slices.SortedFunc(
-				maps.Values(certVolumeMounts),
-				func(a corev1.VolumeMount, b corev1.VolumeMount) int {
-					return cmp.Compare(a.Name, b.Name)
-				},
-			),
+			func() []corev1.VolumeMount {
+				volumeMounts := make([]corev1.VolumeMount, 0, len(certVolumeMounts))
+				for _, name := range slices.Sorted(maps.Keys(certVolumeMounts)) {
+					volumeMounts = append(volumeMounts, corev1.VolumeMount{
+						Name:      name,
+						MountPath: certVolumeMounts[name],
+					})
+				}
+				return volumeMounts
+			}(),
 		)
 	}
 }

internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go

@@ -173,7 +173,7 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 		},
 	}
 
-	bundle := &bundle.RegistryV1{
+	b := &bundle.RegistryV1{
 		CSV: MakeCSV(
 			WithWebhookDefinitions(
 				v1alpha1.WebhookDescription{
@@ -189,31 +189,39 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 							Spec: corev1.PodSpec{
 								Volumes: []corev1.Volume{
 									{
-										Name: "apiservice-cert",
+										Name: "some-other-mount",
 										VolumeSource: corev1.VolumeSource{
 											EmptyDir: &corev1.EmptyDirVolumeSource{},
 										},
 									},
+									// this volume should be replaced by the webhook-cert volume
+									// because it has a volume mount targeting the protected path
+									// /tmp/k8s-webhook-server/serving-certs
 									{
-										Name: "some-other-mount",
+										Name: "some-webhook-cert-mount",
 										VolumeSource: corev1.VolumeSource{
 											EmptyDir: &corev1.EmptyDirVolumeSource{},
 										},
 									},
-									// expect webhook-cert volume to be injected
 								},
 								Containers: []corev1.Container{
 									{
 										Name: "container-1",
 										VolumeMounts: []corev1.VolumeMount{
-											// expect apiservice-cert volume to be injected
+											// the mount path for this volume mount will be replaced with
+											// /tmp/k8s-webhook-server/serving-certs
 											{
 												Name:      "webhook-cert",
 												MountPath: "/webhook-cert-path",
 											}, {
 												Name:      "some-other-mount",
 												MountPath: "/some/other/mount/path",
 											},
+											// this volume mount will be removed
+											{
+												Name:      "some-webhook-cert-mount",
+												MountPath: "/tmp/k8s-webhook-server/serving-certs",
+											},
 										},
 									},
 									{
@@ -229,7 +237,7 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 		),
 	}
 
-	objs, err := generators.BundleCSVDeploymentGenerator(bundle, render.Options{
+	objs, err := generators.BundleCSVDeploymentGenerator(b, render.Options{
 		InstallNamespace:    "install-namespace",
 		CertificateProvider: fakeProvider,
 	})
@@ -247,23 +255,6 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 			},
 		},
 		{
-			Name: "apiservice-cert",
-			VolumeSource: corev1.VolumeSource{
-				Secret: &corev1.SecretVolumeSource{
-					SecretName: "some-secret",
-					Items: []corev1.KeyToPath{
-						{
-							Key:  "some-cert-key",
-							Path: "apiserver.crt",
-						},
-						{
-							Key:  "some-private-key-key",
-							Path: "apiserver.key",
-						},
-					},
-				},
-			},
-		}, {
 			Name: "webhook-cert",
 			VolumeSource: corev1.VolumeSource{
 				Secret: &corev1.SecretVolumeSource{
@@ -290,10 +281,6 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 					Name:      "some-other-mount",
 					MountPath: "/some/other/mount/path",
 				},
-				{
-					Name:      "apiservice-cert",
-					MountPath: "/apiserver.local.config/certificates",
-				},
 				{
 					Name:      "webhook-cert",
 					MountPath: "/tmp/k8s-webhook-server/serving-certs",
@@ -303,10 +290,6 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 		{
 			Name: "container-2",
 			VolumeMounts: []corev1.VolumeMount{
-				{
-					Name:      "apiservice-cert",
-					MountPath: "/apiserver.local.config/certificates",
-				},
 				{
 					Name:      "webhook-cert",
 					MountPath: "/tmp/k8s-webhook-server/serving-certs",