From 16f51af57339077e30f95ebb4654730505b39775 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Tue, 14 Sep 2021 16:02:46 -0700 Subject: [PATCH] lib/resourcemerge/core: Remove unrecognized volumes and mounts Since this package was created in d9f6718de0 (lib: add lib for applying objects, 2018-08-14, #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 02bb9ba829 (lib/resourcemerge/core: Clear env and envFrom if unset in manifest, 2021-04-20, #549) and ca299b8871 (lib/resourcemerge: remove ports which are no longer required, 2020-02-13, #322). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2002834 [2]: https://github.com/openshift/cluster-autoscaler-operator/commit/f08589d4767037636fc0d3e78b40d61f9eed8104#diff-547486373183980619528df695869ed32b80c18383bc16b57a5ee931bf0edd39L89 [3]: https://github.com/openshift/cluster-autoscaler-operator/commit/9a7b3be569980316235990fe1feb106f0155e4b2#diff-d0cf785e044c611986a4d9bdd65bb373c86f9eb1c97bd3f105062184342a872dR4 --- lib/resourcemerge/core.go | 97 +++++++++++++------- lib/resourcemerge/core_test.go | 161 +++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 35 deletions(-) diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go index 01a2a7444..7d0395c95 100644 --- 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,6 +314,36 @@ 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 @@ -354,6 +351,36 @@ func ensureVolumeMount(modified *bool, existing *corev1.VolumeMount, required co } } +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 index 9939b00c3..91c2e13c8 100644 --- 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 {