Repository: operator-framework/operator-controller
Pull request #2002 (2025-06-02 11:27:38)

:bug: Fix webhook cert duplicate volume mount path bug
# Description

1. Removes the APIService volume mount - since we don't yet support APIServices (let's avoid adding things that aren't needed yet)
2. Moves the tls.crt and tls.key magic strings to constants
3. Addresses the following bug:

When installing the Red Hat Network Observability operator, we were faced with the following error:

```
"error for resolved bundle \"network-observability-operator.v1.7.0\" with version \"1.7.0\": failed to create resource: Deployment.apps \"netobserv-controller-manager\" is invalid: spec.template.spec.containers[0].volumeMounts[2].mountPath: Invalid value: \"/tmp/k8s-webhook-server/serving-certs\": must be unique",
```

The root cause is related to the operator's `Deployment`'s `Pod` template spec described in the CSV that included a volume mount pointing to the webhook certificate volume mount path (but had a different name than 'webhook-cert'):

```
volumeMounts:
- mountPath: /tmp/k8s-webhook-server/serving-certs
  name: cert
  readOnly: true
```

This PR updates the bundle deployment generator to replace any volume/mount referencing a protected path with the OLM generated values.

Before fixes, OLMv1 generated a deployment like:

```
volumeMounts:
- mountPath: /tmp/k8s-webhook-server/serving-certs
  name: cert
- mountPath: /etc/tls/private
  name: manager-metric-tls
  readOnly: true
- mountPath: /apiserver.local.config/certificates
  name: apiservice-cert
- mountPath: /tmp/k8s-webhook-server/serving-certs
  name: webhook-cert
```

```
volumes:
- name: cert
  secret:
    defaultMode: 420
    secretName: webhook-server-cert
- name: manager-metric-tls
  secret:
    defaultMode: 420
    secretName: manager-metrics-tls
- name: apiservice-cert
  secret:
    items:
    - key: tls.crt
      path: apiserver.crt
    - key: tls.key
      path: apiserver.key
    secretName: netobserv-controller-manager-service-cert
- name: webhook-cert
  secret:
    items:
    - key: tls.crt
      path: tls.crt
    - key: tls.key
      path: tls.key
    secretName: netobserv-controller-manager-service-cert
```
 
Now, it looks like:

```
volumeMounts:
- mountPath: /etc/tls/private
  name: manager-metric-tls
  readOnly: true
- mountPath: /tmp/k8s-webhook-server/serving-certs
  name: webhook-cert
```

```
volumes:
- name: manager-metric-tls
  secret:
    defaultMode: 420
    secretName: manager-metrics-tls
- name: webhook-cert
  secret:
    items:
    - key: tls.crt
      path: tls.crt
    - key: tls.key
      path: tls.key
    secretName: netobserv-controller-manager-service-cert
```

## Reviewer Checklist

- [ ] API Go Documentation
- [ ] Tests: Unit Tests (and E2E Tests, if appropriate)
- [ ] Comprehensive Commit Messages
- [ ] Links to related GitHub Issue(s)


netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *olmv1* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 91dc9a199f927f68f129143552738e8ee3420052 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/olmv1/deploys/683d8aad1924f40008794ad1 |
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-controller/pull/2002#pullrequestreview-2887973230" title="Approved">camilamacedo86</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=operator-framework%2Foperator-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/operator-framework/operator-controller/blob/main/OWNERS)~~ [camilamacedo86]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators.go
index bdeb85b..7ae8de8 100644
--- a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go
+++ b/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,31 +479,23 @@ 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{
@@ -512,11 +503,11 @@ func addCertVolumesToDeployment(dep *appsv1.Deployment, certSecretInfo render.Ce
 						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
+			}(),
 		)
 	}
 }


Test patch:
diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go
index f2e542d..d0af7f7 100644
--- a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go
+++ b/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,24 +189,27 @@ 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",
@@ -214,6 +217,11 @@ func Test_BundleCSVDeploymentGenerator_WithCertWithCertProvider_Succeeds(t *test
 												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",