lib/resourcemerge/core: Remove unrecognized volumes and mounts

wking · wking · commit 83faa6e716dc · 2021-09-14T16:53:52.000-07:00
Since this package was created in d9f6718 (lib: add lib for applying objects, 2018-08-14, openshift#7), the volume(mount) merge logic has required manifest entries to exist, but has allowed in-cluster entries to persist without removal. That hasn't been a problem until [1]: 1. In 4.3, the autoscaler asked for a ca-cert volume mount, based on the cluster-autoscaler-operator-ca config map. 2. In 4.4, the autoscaler dropped those manifest entries [2]. 3. In 4.9, the autoscaler asked the CVO to remove the config map [3]. That lead some born-in 4.3 clusters to have crashlooping autoscalers, because the mount attempts kept failing on the missing config map. We couldn't think of a plausible reason why cluster admins would want to inject additional volume mounts in a CVO-managed pod configuration, so this commit removes that ability and begins clearing away any volume(mount) configuration that is not present in the reconciling manifest. Cluster administrators who do need to add additional mounts in an emergency are free to use ClusterVersion's spec.overrides to take control of a particular CVO-managed resource. This joins a series of similar previous tightenings, including 02bb9ba (lib/resourcemerge/core: Clear env and envFrom if unset in manifest, 2021-04-20, openshift#549) and ca299b8 (lib/resourcemerge: remove ports which are no longer required, 2020-02-13, openshift#322). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2002834 [2]: openshift/cluster-autoscaler-operator@f08589d#diff-547486373183980619528df695869ed32b80c18383bc16b57a5ee931bf0edd39L89 [3]: openshift/cluster-autoscaler-operator@9a7b3be#diff-d0cf785e044c611986a4d9bdd65bb373c86f9eb1c97bd3f105062184342a872dR4
diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go
@@ -36,23 +36,7 @@ func ensurePodTemplateSpec(modified *bool, existing *corev1.PodTemplateSpec, req
 func ensurePodSpec(modified *bool, existing *corev1.PodSpec, required corev1.PodSpec) {
 	ensureContainers(modified, &existing.InitContainers, required.InitContainers, required.HostNetwork)
 	ensureContainers(modified, &existing.Containers, required.Containers, required.HostNetwork)
-
-	// any volume we specify, we require.
-	for _, required := range required.Volumes {
-		var existingCurr *corev1.Volume
-		for j, curr := range existing.Volumes {
-			if curr.Name == required.Name {
-				existingCurr = &existing.Volumes[j]
-				break
-			}
-		}
-		if existingCurr == nil {
-			*modified = true
-			existing.Volumes = append(existing.Volumes, corev1.Volume{})
-			existingCurr = &existing.Volumes[len(existing.Volumes)-1]
-		}
-		ensureVolume(modified, existingCurr, required)
-	}
+	ensureVolumes(modified, &existing.Volumes, required.Volumes)
 
 	if len(required.RestartPolicy) > 0 {
 		if existing.RestartPolicy != required.RestartPolicy {
@@ -118,24 +102,7 @@ func ensureContainer(modified *bool, existing *corev1.Container, required corev1
 	setStringIfSet(modified, &existing.WorkingDir, required.WorkingDir)
 	ensureResourceRequirements(modified, &existing.Resources, required.Resources)
 	ensureContainerPorts(modified, &existing.Ports, required.Ports, hostNetwork)
-
-	// any volume mount we specify, we require
-	for _, required := range required.VolumeMounts {
-		var existingCurr *corev1.VolumeMount
-		for j, curr := range existing.VolumeMounts {
-			if curr.Name == required.Name {
-				existingCurr = &existing.VolumeMounts[j]
-				break
-			}
-		}
-		if existingCurr == nil {
-			*modified = true
-			existing.VolumeMounts = append(existing.VolumeMounts, corev1.VolumeMount{})
-			existingCurr = &existing.VolumeMounts[len(existing.VolumeMounts)-1]
-		}
-		ensureVolumeMount(modified, existingCurr, required)
-	}
-
+	ensureVolumeMounts(modified, &existing.VolumeMounts, required.VolumeMounts)
 	ensureProbePtr(modified, &existing.LivenessProbe, required.LivenessProbe)
 	ensureProbePtr(modified, &existing.ReadinessProbe, required.ReadinessProbe)
 
@@ -347,13 +314,73 @@ func ensureServicePortDefaults(servicePort *corev1.ServicePort) {
 	}
 }
 
+func ensureVolumeMounts(modified *bool, existing *[]corev1.VolumeMount, required []corev1.VolumeMount) {
+	// any volume mount we specify, we require
+	exists := struct{}{}
+	requiredNames := make(map[string]struct{}, len(required))
+	for _, requiredVolumeMount := range required {
+		requiredNames[requiredVolumeMount.Name] = exists
+		var existingCurr *corev1.VolumeMount
+		for j, curr := range *existing {
+			if curr.Name == requiredVolumeMount.Name {
+				existingCurr = &(*existing)[j]
+				break
+			}
+		}
+		if existingCurr == nil {
+			*modified = true
+			*existing = append(*existing, corev1.VolumeMount{})
+			existingCurr = &(*existing)[len(*existing)-1]
+		}
+		ensureVolumeMount(modified, existingCurr, requiredVolumeMount)
+	}
+
+	// any unrecognized volume mount, we remove
+	for eidx := len(*existing) - 1; eidx >= 0; eidx-- {
+		if _, ok := requiredNames[(*existing)[eidx].Name]; !ok {
+			*modified = true
+			*existing = append((*existing)[:eidx], (*existing)[eidx+1:]...)
+		}
+	}
+}
+
 func ensureVolumeMount(modified *bool, existing *corev1.VolumeMount, required corev1.VolumeMount) {
 	if !equality.Semantic.DeepEqual(required, *existing) {
 		*modified = true
 		*existing = required
 	}
 }
 
+func ensureVolumes(modified *bool, existing *[]corev1.Volume, required []corev1.Volume) {
+	// any volume we specify, we require.
+	exists := struct{}{}
+	requiredNames := make(map[string]struct{}, len(required))
+	for _, requiredVolume := range required {
+		requiredNames[requiredVolume.Name] = exists
+		var existingCurr *corev1.Volume
+		for j, curr := range *existing {
+			if curr.Name == requiredVolume.Name {
+				existingCurr = &(*existing)[j]
+				break
+			}
+		}
+		if existingCurr == nil {
+			*modified = true
+			*existing = append(*existing, corev1.Volume{})
+			existingCurr = &(*existing)[len(*existing)-1]
+		}
+		ensureVolume(modified, existingCurr, requiredVolume)
+	}
+
+	// any unrecognized volume mount, we remove
+	for eidx := len(*existing) - 1; eidx >= 0; eidx-- {
+		if _, ok := requiredNames[(*existing)[eidx].Name]; !ok {
+			*modified = true
+			*existing = append((*existing)[:eidx], (*existing)[eidx+1:]...)
+		}
+	}
+}
+
 func ensureVolume(modified *bool, existing *corev1.Volume, required corev1.Volume) {
 	if pointer.AllPtrFieldsNil(&required.VolumeSource) {
 		required.VolumeSource = corev1.VolumeSource{
diff --git a/lib/resourcemerge/core_test.go b/lib/resourcemerge/core_test.go
@@ -451,6 +451,167 @@ func TestEnsurePodSpec(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "add volumes on container",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{Name: "test"},
+				},
+			},
+			input: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							EmptyDir: &corev1.EmptyDirVolumeSource{},
+						},
+					},
+				},
+			},
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							EmptyDir: &corev1.EmptyDirVolumeSource{},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "modify volumes on container",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt/a",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							EmptyDir: &corev1.EmptyDirVolumeSource{},
+						},
+					},
+				},
+			},
+			input: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt/b",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							ConfigMap: &corev1.ConfigMapVolumeSource{
+								LocalObjectReference: corev1.LocalObjectReference{
+									Name: "test-config-map",
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt/b",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							ConfigMap: &corev1.ConfigMapVolumeSource{
+								LocalObjectReference: corev1.LocalObjectReference{
+									Name: "test-config-map",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "remove container volumes",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Name: "test",
+						VolumeMounts: []corev1.VolumeMount{
+							{
+								Name:      "test-volume",
+								MountPath: "/mnt",
+							},
+						},
+					},
+				},
+				Volumes: []corev1.Volume{
+					{
+						Name: "test-volume",
+						VolumeSource: corev1.VolumeSource{
+							EmptyDir: &corev1.EmptyDirVolumeSource{},
+						},
+					},
+				},
+			},
+			input: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{Name: "test"},
+				},
+			},
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{Name: "test"},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
