From 7708bcb4fc6dad3a18f7e11e8e9e0ccb37b8f321 Mon Sep 17 00:00:00 2001 From: Nikola Date: Sat, 18 Nov 2023 17:14:39 +0200 Subject: [PATCH 1/5] add warnings for duplicated paths in projected volumes --- pkg/api/pod/warnings.go | 54 ++++++++++++++++++++++++ pkg/api/pod/warnings_test.go | 80 ++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 8958aa51a47..b72e515cd4d 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -170,6 +170,60 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } + items := sets.NewString() + for _, v := range podSpec.Volumes { + if v.Projected != nil { + for _, k := range v.Projected.Sources { + if k.ServiceAccountToken != nil { + items.Insert(k.ServiceAccountToken.Path) + } + } + } + + } + + if items.Len() > 0 { + for i, item := range podSpec.Volumes { + if item.Projected != nil { + for _, k := range item.Projected.Sources { + if k.ConfigMap != nil { + if len(k.ConfigMap.Items) > 0 { + for j, item := range k.ConfigMap.Items { + if items.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "configMap").Index(j), item.Path)) + } + } + } + } + + if k.Secret != nil { + if len(k.Secret.Items) > 0 { + for j, item := range k.Secret.Items { + if items.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "secrets").Index(j), item.Path)) + } + } + } + } + + if k.DownwardAPI != nil { + for j, item := range k.DownwardAPI.Items { + if items.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "downwardAPI").Index(j), item.Path)) + } + } + } + + if k.ClusterTrustBundle != nil { + if items.Has(k.ClusterTrustBundle.Path) { + warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "ClusterTrustBundle"), k.ClusterTrustBundle.Path)) + } + } + } + } + } + } + // duplicate hostAliases (#91670, #58477) if len(podSpec.HostAliases) > 1 { items := sets.New[string]() diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 6405977b7cb..bea4dd18114 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -235,6 +235,86 @@ func TestWarnings(t *testing.T) { }, expected: []string{`spec.volumes[0].rbd: deprecated in v1.28, non-functional in v1.31+`}, }, + { + name: "duplicated path in projected volumes - secret and service account token", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "t", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {Secret: &api.SecretProjection{Items: []api.KeyToPath{{Path: "/test-path", Key: "test"}}}}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, + }}, + }, + }, + }, + }}, + expected: []string{ + "spec.volumes[0].projected.sources.secrets[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + }, + }, + { + name: "duplicated path in projected volumes - configMap and service account token", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "t", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ConfigMap: &api.ConfigMapProjection{Items: []api.KeyToPath{{Path: "/test-path", Key: "test"}}}}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, + }}, + }, + }, + }, + }}, + expected: []string{ + "spec.volumes[0].projected.sources.configMap[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + }, + }, + { + name: "duplicated path in projected volumes - downwardAPI and service account token", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "t", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {DownwardAPI: &api.DownwardAPIProjection{Items: []api.DownwardAPIVolumeFile{{Path: "/test-path"}}}}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, + }}, + }, + }, + }, + }}, + expected: []string{ + "spec.volumes[0].projected.sources.downwardAPI[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + }, + }, + { + name: "duplicated path in projected volumes - ClusterTrustBundle and service account token", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "t", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ClusterTrustBundle: &api.ClusterTrustBundleProjection{Path: "/test-path"}}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, + }}, + }, + }, + }, + }}, + expected: []string{ + "spec.volumes[0].projected.sources.ClusterTrustBundle: has duplicated path with ServiceAccountToken \"/test-path\"", + }, + }, { name: "duplicate hostAlias", template: &api.PodTemplateSpec{Spec: api.PodSpec{ From 30c325a3dc87a43746e70dc84d319bc23dde7c3d Mon Sep 17 00:00:00 2001 From: Nikola Date: Sun, 12 May 2024 16:21:39 +0300 Subject: [PATCH 2/5] add validation for configmap, secret and downward api --- pkg/api/pod/warnings.go | 134 +++++++++++++++++++------------ pkg/api/pod/warnings_test.go | 148 ++++++++++++++++++++++++++++------- 2 files changed, 202 insertions(+), 80 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index b72e515cd4d..ea05b82ba31 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -170,58 +170,14 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } - items := sets.NewString() - for _, v := range podSpec.Volumes { - if v.Projected != nil { - for _, k := range v.Projected.Sources { - if k.ServiceAccountToken != nil { - items.Insert(k.ServiceAccountToken.Path) - } - } - } - + overlappingPathInContainers := warningsForOverlappingVirtualPaths(podSpec) + if len(overlappingPathInContainers) > 0 { + warnings = append(warnings, overlappingPathInContainers...) } - if items.Len() > 0 { - for i, item := range podSpec.Volumes { - if item.Projected != nil { - for _, k := range item.Projected.Sources { - if k.ConfigMap != nil { - if len(k.ConfigMap.Items) > 0 { - for j, item := range k.ConfigMap.Items { - if items.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "configMap").Index(j), item.Path)) - } - } - } - } - - if k.Secret != nil { - if len(k.Secret.Items) > 0 { - for j, item := range k.Secret.Items { - if items.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "secrets").Index(j), item.Path)) - } - } - } - } - - if k.DownwardAPI != nil { - for j, item := range k.DownwardAPI.Items { - if items.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "downwardAPI").Index(j), item.Path)) - } - } - } - - if k.ClusterTrustBundle != nil { - if items.Has(k.ClusterTrustBundle.Path) { - warnings = append(warnings, fmt.Sprintf("%s: has duplicated path with ServiceAccountToken %q", fieldPath.Child("spec", "volumes").Index(i).Child("projected", "sources", "ClusterTrustBundle"), k.ClusterTrustBundle.Path)) - } - } - } - } - } + overlappingPathInInitContainers := warningsForOverlappingVirtualPaths(podSpec) + if len(overlappingPathInInitContainers) > 0 { + warnings = append(warnings, overlappingPathInInitContainers...) } // duplicate hostAliases (#91670, #58477) @@ -411,3 +367,81 @@ func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fi } return warnings } + +func warningsForOverlappingVirtualPaths(podSpec *api.PodSpec) []string { + warnings := make([]string, 0) + for _, v := range podSpec.Volumes { + if v.ConfigMap != nil && v.ConfigMap.Items != nil { + a := sets.NewString() + for _, item := range v.ConfigMap.Items { + if a.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("config map: %q - conflicting duplicate paths", v.ConfigMap.Name)) + } + a.Insert(item.Path) + } + } + + if v.Secret != nil && v.Secret.Items != nil { + a := sets.NewString() + for _, item := range v.Secret.Items { + if a.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("secret: %q - conflicting duplicate paths", v.Secret.SecretName)) + } + a.Insert(item.Path) + } + } + + if v.DownwardAPI != nil && v.DownwardAPI.Items != nil { + a := sets.NewString() + for _, item := range v.DownwardAPI.Items { + if a.Has(item.Path) { + warnings = append(warnings, "downward api - conflicting duplicate paths") + } + a.Insert(item.Path) + } + } + + if v.Projected != nil { + a := sets.NewString() + for _, item := range v.Projected.Sources { + if item.ServiceAccountToken != nil { + a.Insert(item.ServiceAccountToken.Path) + } + } + + for _, item := range v.Projected.Sources { + if item.ConfigMap != nil && item.ConfigMap.Items != nil { + for _, item := range item.ConfigMap.Items { + if a.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) + } + } + } + + if item.Secret != nil && item.Secret.Items != nil { + for _, item := range item.Secret.Items { + if a.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) + } + } + } + + if item.DownwardAPI != nil && item.DownwardAPI.Items != nil { + for _, item := range item.DownwardAPI.Items { + if a.Has(item.Path) { + warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) + } + } + } + + if item.ClusterTrustBundle != nil { + if a.Has(item.ClusterTrustBundle.Path) { + warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) + } + } + } + } + + } + return warnings +} diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index bea4dd18114..9fd678c4f14 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -236,83 +236,171 @@ func TestWarnings(t *testing.T) { expected: []string{`spec.volumes[0].rbd: deprecated in v1.28, non-functional in v1.31+`}, }, { - name: "duplicated path in projected volumes - secret and service account token", + name: "overlapping paths in a configmap volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { - Name: "t", VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - {Secret: &api.SecretProjection{Items: []api.KeyToPath{{Path: "/test-path", Key: "test"}}}}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, - }}, + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test"}, + }, + }, }, }, }, }}, expected: []string{ - "spec.volumes[0].projected.sources.secrets[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + "config map: \"foo\" - conflicting duplicate paths", }, }, { - name: "duplicated path in projected volumes - configMap and service account token", + name: "overlapping paths in a secret volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { - Name: "t", VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - {ConfigMap: &api.ConfigMapProjection{Items: []api.KeyToPath{{Path: "/test-path", Key: "test"}}}}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, - }}, + Secret: &api.SecretVolumeSource{ + SecretName: "foo", + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test"}, + }, + }, }, }, }, }}, expected: []string{ - "spec.volumes[0].projected.sources.configMap[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + "secret: \"foo\" - conflicting duplicate paths", }, }, { - name: "duplicated path in projected volumes - downwardAPI and service account token", + name: "overlapping paths in a downward api volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { - Name: "t", VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - {DownwardAPI: &api.DownwardAPIProjection{Items: []api.DownwardAPIVolumeFile{{Path: "/test-path"}}}}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, - }}, + DownwardAPI: &api.DownwardAPIVolumeSource{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name\n"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.labels\n"}, Path: "test"}, + }, + }, }, }, }, }}, expected: []string{ - "spec.volumes[0].projected.sources.downwardAPI[0]: has duplicated path with ServiceAccountToken \"/test-path\"", + "downward api - conflicting duplicate paths", }, }, { - name: "duplicated path in projected volumes - ClusterTrustBundle and service account token", + name: "overlapping paths in projected volume volume - service account and config", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { - Name: "t", + Name: "foo", VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ClusterTrustBundle: &api.ClusterTrustBundleProjection{Path: "/test-path"}}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{Path: "/test-path"}}, - }}, + {ConfigMap: &api.ConfigMapProjection{ + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, }, }, }, }}, expected: []string{ - "spec.volumes[0].projected.sources.ClusterTrustBundle: has duplicated path with ServiceAccountToken \"/test-path\"", + "projected volume: \"foo\" - conflicting duplicate paths", + }, + }, + { + name: "overlapping paths in projected volume volume - service account and secret", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {Secret: &api.SecretProjection{ + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "projected volume: \"foo\" - conflicting duplicate paths", + }, + }, + { + name: "overlapping paths in projected volume volume - service account and downward api", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name\n"}, Path: "test"}, + }, + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "projected volume: \"foo\" - conflicting duplicate paths", + }, + }, + { + name: "overlapping paths in projected volume volume - service account and cluster trust bundle", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + Path: "test", + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "projected volume: \"foo\" - conflicting duplicate paths", }, }, { From 205a026bb192868955e732f321d09fd909b61947 Mon Sep 17 00:00:00 2001 From: Nikola Date: Wed, 3 Jul 2024 11:50:46 +0300 Subject: [PATCH 3/5] extend pod warning rules for overlapping paths --- pkg/api/pod/warnings.go | 166 +++++++++++------ pkg/api/pod/warnings_test.go | 343 +++++++++++++++++++++++++++++++++-- 2 files changed, 439 insertions(+), 70 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index ea05b82ba31..58f80ac9813 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -19,6 +19,8 @@ package pod import ( "context" "fmt" + "os" + "sort" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -170,16 +172,11 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } - overlappingPathInContainers := warningsForOverlappingVirtualPaths(podSpec) + overlappingPathInContainers := warningsForOverlappingVirtualPaths(podSpec.Volumes) if len(overlappingPathInContainers) > 0 { warnings = append(warnings, overlappingPathInContainers...) } - overlappingPathInInitContainers := warningsForOverlappingVirtualPaths(podSpec) - if len(overlappingPathInInitContainers) > 0 { - warnings = append(warnings, overlappingPathInInitContainers...) - } - // duplicate hostAliases (#91670, #58477) if len(podSpec.HostAliases) > 1 { items := sets.New[string]() @@ -368,76 +365,84 @@ func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fi return warnings } -func warningsForOverlappingVirtualPaths(podSpec *api.PodSpec) []string { +// warningsForOverlappingVirtualPaths validates that there are no overlapping paths in single ConfigMapVolume, SecretVolume, DownwardAPIVolume and ProjectedVolume. +// A volume can try to load different keys to the same path which will result in overwriting of the value from the latest registered key +// Another possible scenario is when one of the path contains the other key path. Example: +// configMap: +// +// name: myconfig +// items: +// - key: key1 +// path: path +// - key: key2 +// path: path/path2 +// +// In such cases we either get `is directory` or 'file exists' error message. +func warningsForOverlappingVirtualPaths(volumes []api.Volume) []string { warnings := make([]string, 0) - for _, v := range podSpec.Volumes { + + for _, v := range volumes { if v.ConfigMap != nil && v.ConfigMap.Items != nil { - a := sets.NewString() - for _, item := range v.ConfigMap.Items { - if a.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("config map: %q - conflicting duplicate paths", v.ConfigMap.Name)) - } - a.Insert(item.Path) + w := checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items), fmt.Sprintf("volume %q (ConfigMap %q): overlapping path", v.Name, v.ConfigMap.Name)) + if len(w) > 0 { + warnings = append(warnings, w...) } } if v.Secret != nil && v.Secret.Items != nil { - a := sets.NewString() - for _, item := range v.Secret.Items { - if a.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("secret: %q - conflicting duplicate paths", v.Secret.SecretName)) - } - a.Insert(item.Path) + w := checkVolumeMappingForOverlap(extractPaths(v.Secret.Items), fmt.Sprintf("volume %q (Secret %q): overlapping path", v.Name, v.Secret.SecretName)) + if len(w) > 0 { + warnings = append(warnings, w...) } } if v.DownwardAPI != nil && v.DownwardAPI.Items != nil { - a := sets.NewString() - for _, item := range v.DownwardAPI.Items { - if a.Has(item.Path) { - warnings = append(warnings, "downward api - conflicting duplicate paths") - } - a.Insert(item.Path) + w := checkVolumeMappingForOverlap(extractPathsDownwardAPI(v.DownwardAPI.Items), fmt.Sprintf("volume %q (DownwardAPI): overlapping path", v.Name)) + if len(w) > 0 { + warnings = append(warnings, w...) } } if v.Projected != nil { - a := sets.NewString() - for _, item := range v.Projected.Sources { - if item.ServiceAccountToken != nil { - a.Insert(item.ServiceAccountToken.Path) - } - } + var sourcePaths, allPaths []string + var errorMessage string - for _, item := range v.Projected.Sources { - if item.ConfigMap != nil && item.ConfigMap.Items != nil { - for _, item := range item.ConfigMap.Items { - if a.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) - } + for _, source := range v.Projected.Sources { + switch { + case source.ConfigMap != nil && source.ConfigMap.Items != nil: + sourcePaths = extractPaths(source.ConfigMap.Items) + errorMessage = fmt.Sprintf("volume %q (Projected ConfigMap %q): overlapping path", v.Name, source.ConfigMap.Name) + case source.Secret != nil && source.Secret.Items != nil: + sourcePaths = extractPaths(source.Secret.Items) + errorMessage = fmt.Sprintf("volume %q (Projected Secret %q): overlapping path", v.Name, source.Secret.Name) + case source.DownwardAPI != nil && source.DownwardAPI.Items != nil: + sourcePaths = extractPathsDownwardAPI(source.DownwardAPI.Items) + errorMessage = fmt.Sprintf("volume %q (Projected DownwardAPI): overlapping path", v.Name) + case source.ServiceAccountToken != nil: + sourcePaths = []string{source.ServiceAccountToken.Path} + errorMessage = fmt.Sprintf("volume %q (Projected ServiceAccountToken): overlapping path", v.Name) + case source.ClusterTrustBundle != nil: + sourcePaths = []string{source.ClusterTrustBundle.Path} + if source.ClusterTrustBundle.Name != nil { + errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping path", v.Name, *source.ClusterTrustBundle.Name) + } else { + errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping path", v.Name, *source.ClusterTrustBundle.SignerName) } } - if item.Secret != nil && item.Secret.Items != nil { - for _, item := range item.Secret.Items { - if a.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) - } - } + if len(sourcePaths) == 0 { + continue } - if item.DownwardAPI != nil && item.DownwardAPI.Items != nil { - for _, item := range item.DownwardAPI.Items { - if a.Has(item.Path) { - warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) - } - } + warningsInSource := checkVolumeMappingForOverlap(sourcePaths, errorMessage) + if len(warningsInSource) > 0 { + warnings = append(warnings, warningsInSource...) } - if item.ClusterTrustBundle != nil { - if a.Has(item.ClusterTrustBundle.Path) { - warnings = append(warnings, fmt.Sprintf("projected volume: %q - conflicting duplicate paths", v.Name)) - } + allPaths = append(allPaths, sourcePaths...) + warningsInAllPaths := checkVolumeMappingForOverlap(allPaths, errorMessage) + if len(warningsInAllPaths) > 0 { + warnings = append(warnings, warningsInAllPaths...) } } } @@ -445,3 +450,56 @@ func warningsForOverlappingVirtualPaths(podSpec *api.PodSpec) []string { } return warnings } + +func extractPaths(mapping []api.KeyToPath) []string { + result := make([]string, 0, len(mapping)) + + for _, v := range mapping { + result = append(result, v.Path) + } + return result +} + +func extractPathsDownwardAPI(mapping []api.DownwardAPIVolumeFile) []string { + result := make([]string, 0, len(mapping)) + + for _, v := range mapping { + result = append(result, v.Path) + } + return result +} + +func checkVolumeMappingForOverlap(paths []string, warningMessage string) []string { + pathSeparator := string(os.PathSeparator) + warnings := make([]string, 0) + uniquePaths := sets.New[string]() + + for _, path := range paths { + normalizedPath := strings.TrimRight(path, pathSeparator) + if collision := checkForOverlap(uniquePaths, normalizedPath); collision != nil { + warnings = append(warnings, fmt.Sprintf("%s %q with %q", warningMessage, normalizedPath, *collision)) + } + uniquePaths.Insert(normalizedPath) + } + + return warnings +} + +func checkForOverlap(paths sets.Set[string], path string) *string { + pathSeparator := string(os.PathSeparator) + p := paths.UnsortedList() + sort.Strings(p) + + for _, item := range p { + switch { + case item == path: + return &item + case strings.HasPrefix(item+pathSeparator, path): + return &item + case strings.HasPrefix(path+pathSeparator, item): + return &item + } + } + + return nil +} diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 9fd678c4f14..9a70dc6261e 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -143,6 +143,7 @@ func TestWarnings(t *testing.T) { api.ResourceMemory: resource.MustParse("4m"), api.ResourceEphemeralStorage: resource.MustParse("4m"), } + testName := "Test" testcases := []struct { name string template *api.PodTemplateSpec @@ -240,6 +241,7 @@ func TestWarnings(t *testing.T) { template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { + Name: "Test", VolumeSource: api.VolumeSource{ ConfigMap: &api.ConfigMapVolumeSource{ LocalObjectReference: api.LocalObjectReference{Name: "foo"}, @@ -253,7 +255,51 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "config map: \"foo\" - conflicting duplicate paths", + "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test\" with \"test\"", + }, + }, + { + name: "overlapping paths in a configmap volume - try to mount dir path into a file", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "Test", + VolumeSource: api.VolumeSource{ + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test/app"}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test/app\" with \"test\"", + }, + }, + { + name: "overlapping paths in a configmap volume - try to mount file into a dir path", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "Test", + VolumeSource: api.VolumeSource{ + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "bar", Path: "test/app"}, + {Key: "foo", Path: "test"}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test\" with \"test/app\"", }, }, { @@ -261,6 +307,7 @@ func TestWarnings(t *testing.T) { template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { + Name: "Test", VolumeSource: api.VolumeSource{ Secret: &api.SecretVolumeSource{ SecretName: "foo", @@ -274,7 +321,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "secret: \"foo\" - conflicting duplicate paths", + "volume \"Test\" (Secret \"foo\"): overlapping path \"test\" with \"test\"", }, }, { @@ -282,11 +329,12 @@ func TestWarnings(t *testing.T) { template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { + Name: "Test", VolumeSource: api.VolumeSource{ DownwardAPI: &api.DownwardAPIVolumeSource{ Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name\n"}, Path: "test"}, - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.labels\n"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.labels"}, Path: "test"}, }, }, }, @@ -294,11 +342,11 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "downward api - conflicting duplicate paths", + "volume \"Test\" (DownwardAPI): overlapping path \"test\" with \"test\"", }, }, { - name: "overlapping paths in projected volume volume - service account and config", + name: "overlapping paths in projected volume - service account and config", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -307,6 +355,7 @@ func TestWarnings(t *testing.T) { Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ {ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, Items: []api.KeyToPath{ {Key: "foo", Path: "test"}, }, @@ -321,11 +370,67 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "projected volume: \"foo\" - conflicting duplicate paths", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", }, }, { - name: "overlapping paths in projected volume volume - service account and secret", + name: "overlapping paths in projected volume volume - service account dir and config file", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test/file", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test/file\" with \"test\"", + }, + }, + { + name: "overlapping paths in projected volume - service account file and config dir", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test/file"}, + }, + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test/file\"", + }, + }, + { + name: "overlapping paths in projected volume - service account and secret", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -334,10 +439,12 @@ func TestWarnings(t *testing.T) { Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ {Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, Items: []api.KeyToPath{ {Key: "foo", Path: "test"}, }, - }}, + }, + }, {ServiceAccountToken: &api.ServiceAccountTokenProjection{ Path: "test", }}, @@ -348,11 +455,11 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "projected volume: \"foo\" - conflicting duplicate paths", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", }, }, { - name: "overlapping paths in projected volume volume - service account and downward api", + name: "overlapping paths in projected volume - service account and downward api", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -362,7 +469,7 @@ func TestWarnings(t *testing.T) { Sources: []api.VolumeProjection{ {DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name\n"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, }, }}, {ServiceAccountToken: &api.ServiceAccountTokenProjection{ @@ -375,11 +482,11 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "projected volume: \"foo\" - conflicting duplicate paths", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", }, }, { - name: "overlapping paths in projected volume volume - service account and cluster trust bundle", + name: "overlapping paths in projected volume - service account and cluster trust bundle", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -388,7 +495,7 @@ func TestWarnings(t *testing.T) { Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - Path: "test", + Name: &testName, Path: "test", }}, {ServiceAccountToken: &api.ServiceAccountTokenProjection{ Path: "test", @@ -400,7 +507,178 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "projected volume: \"foo\" - conflicting duplicate paths", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + }, + }, + { + name: "overlapping paths in projected volume - service account and cluster trust bundle with signer name", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test", + }}, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + }, + }, + { + name: "overlapping paths in projected volume - secret and config map", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + { + Key: "mykey", + Path: "test", + }, + }, + }}, + { + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestConfigMap"}, + Items: []api.KeyToPath{ + { + Key: "mykey", + Path: "test/test1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected ConfigMap \"TestConfigMap\"): overlapping path \"test/test1\" with \"test\"", + }, + }, + { + name: "overlapping paths in projected volume - config map and downward api", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + { + Key: "mykey", + Path: "test", + }, + }, + }}, + { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test/test2"}, + }, + }, + }, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected DownwardAPI): overlapping path \"test/test2\" with \"test\"", + }, + }, + { + name: "overlapping paths in projected volume - downward api and downward api", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test/test2"}, + }, + }, + }, + { + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + Name: &testName, Path: "test", + }, + }, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected ClusterTrustBundle \"Test\"): overlapping path \"test\" with \"test/test2\"", + }, + }, + { + name: "overlapping paths in projected volume - multiple sources", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test/test", + }}, + {DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + }, + }}, + {Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, + }, + }, + {ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + "volume \"foo\" (Projected DownwardAPI): overlapping path \"test\" with \"test/test\"", + "volume \"foo\" (Projected Secret \"Test\"): overlapping path \"test\" with \"test\"", + "volume \"foo\" (Projected Secret \"Test\"): overlapping path \"test\" with \"test/test\"", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test/test\"", + "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", }, }, { @@ -1351,3 +1629,36 @@ func TestTemplateOnlyWarnings(t *testing.T) { }) } } + +func TestCheckForOverLap(t *testing.T) { + testCase := map[string]struct { + checkPaths []string + path string + expected string + }{ + "exact match": { + checkPaths: []string{"path/path1"}, + path: "path/path1", + expected: "path/path1", + }, + "between file and dir": { + checkPaths: []string{"path/path1"}, + path: "path", + expected: "path/path1", + }, + "between dir and file": { + checkPaths: []string{"path"}, + path: "path/path1", + expected: "path", + }, + } + + for name, tc := range testCase { + t.Run(name, func(t *testing.T) { + result := checkForOverlap(sets.New(tc.checkPaths...), tc.path) + if *result != tc.expected { + t.Errorf("expected %v, got %v", tc.expected, *result) + } + }) + } +} From 1853742da6212a55cfe41328d8d463efe7c0bd93 Mon Sep 17 00:00:00 2001 From: Nikola Date: Sun, 14 Jul 2024 13:59:16 +0300 Subject: [PATCH 4/5] refactor the duplicate paths warning for pods logic --- pkg/api/pod/warnings.go | 79 ++++---- pkg/api/pod/warnings_test.go | 377 ++++++++++++++++++++++++----------- 2 files changed, 302 insertions(+), 154 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 58f80ac9813..45ac5ec042a 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -172,9 +172,8 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } - overlappingPathInContainers := warningsForOverlappingVirtualPaths(podSpec.Volumes) - if len(overlappingPathInContainers) > 0 { - warnings = append(warnings, overlappingPathInContainers...) + if overlaps := warningsForOverlappingVirtualPaths(podSpec.Volumes); len(overlaps) > 0 { + warnings = append(warnings, overlaps...) } // duplicate hostAliases (#91670, #58477) @@ -379,54 +378,50 @@ func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fi // // In such cases we either get `is directory` or 'file exists' error message. func warningsForOverlappingVirtualPaths(volumes []api.Volume) []string { - warnings := make([]string, 0) + var warnings []string for _, v := range volumes { if v.ConfigMap != nil && v.ConfigMap.Items != nil { - w := checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items), fmt.Sprintf("volume %q (ConfigMap %q): overlapping path", v.Name, v.ConfigMap.Name)) - if len(w) > 0 { - warnings = append(warnings, w...) - } + warnings = append(warnings, checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items), fmt.Sprintf("volume %q (ConfigMap %q): overlapping paths", v.Name, v.ConfigMap.Name))...) } if v.Secret != nil && v.Secret.Items != nil { - w := checkVolumeMappingForOverlap(extractPaths(v.Secret.Items), fmt.Sprintf("volume %q (Secret %q): overlapping path", v.Name, v.Secret.SecretName)) - if len(w) > 0 { - warnings = append(warnings, w...) - } + warnings = append(warnings, checkVolumeMappingForOverlap(extractPaths(v.Secret.Items), fmt.Sprintf("volume %q (Secret %q): overlapping paths", v.Name, v.Secret.SecretName))...) } if v.DownwardAPI != nil && v.DownwardAPI.Items != nil { - w := checkVolumeMappingForOverlap(extractPathsDownwardAPI(v.DownwardAPI.Items), fmt.Sprintf("volume %q (DownwardAPI): overlapping path", v.Name)) - if len(w) > 0 { - warnings = append(warnings, w...) - } + warnings = append(warnings, checkVolumeMappingForOverlap(extractPathsDownwardAPI(v.DownwardAPI.Items), fmt.Sprintf("volume %q (DownwardAPI): overlapping paths", v.Name))...) } if v.Projected != nil { - var sourcePaths, allPaths []string + var sourcePaths []string var errorMessage string + allPaths := sets.New[string]() for _, source := range v.Projected.Sources { + if source == (api.VolumeProjection{}) { + warnings = append(warnings, fmt.Sprintf("volume %q (Projected) has no sources provided", v.Name)) + continue + } switch { case source.ConfigMap != nil && source.ConfigMap.Items != nil: sourcePaths = extractPaths(source.ConfigMap.Items) - errorMessage = fmt.Sprintf("volume %q (Projected ConfigMap %q): overlapping path", v.Name, source.ConfigMap.Name) + errorMessage = fmt.Sprintf("volume %q (Projected ConfigMap %q): overlapping paths", v.Name, source.ConfigMap.Name) case source.Secret != nil && source.Secret.Items != nil: sourcePaths = extractPaths(source.Secret.Items) - errorMessage = fmt.Sprintf("volume %q (Projected Secret %q): overlapping path", v.Name, source.Secret.Name) + errorMessage = fmt.Sprintf("volume %q (Projected Secret %q): overlapping paths", v.Name, source.Secret.Name) case source.DownwardAPI != nil && source.DownwardAPI.Items != nil: sourcePaths = extractPathsDownwardAPI(source.DownwardAPI.Items) - errorMessage = fmt.Sprintf("volume %q (Projected DownwardAPI): overlapping path", v.Name) + errorMessage = fmt.Sprintf("volume %q (Projected DownwardAPI): overlapping paths", v.Name) case source.ServiceAccountToken != nil: sourcePaths = []string{source.ServiceAccountToken.Path} - errorMessage = fmt.Sprintf("volume %q (Projected ServiceAccountToken): overlapping path", v.Name) + errorMessage = fmt.Sprintf("volume %q (Projected ServiceAccountToken): overlapping paths", v.Name) case source.ClusterTrustBundle != nil: sourcePaths = []string{source.ClusterTrustBundle.Path} if source.ClusterTrustBundle.Name != nil { - errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping path", v.Name, *source.ClusterTrustBundle.Name) + errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping paths", v.Name, *source.ClusterTrustBundle.Name) } else { - errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping path", v.Name, *source.ClusterTrustBundle.SignerName) + errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping paths", v.Name, *source.ClusterTrustBundle.SignerName) } } @@ -434,16 +429,14 @@ func warningsForOverlappingVirtualPaths(volumes []api.Volume) []string { continue } - warningsInSource := checkVolumeMappingForOverlap(sourcePaths, errorMessage) - if len(warningsInSource) > 0 { - warnings = append(warnings, warningsInSource...) - } + warnings = append(warnings, checkVolumeMappingForOverlap(sourcePaths, errorMessage)...) - allPaths = append(allPaths, sourcePaths...) - warningsInAllPaths := checkVolumeMappingForOverlap(allPaths, errorMessage) - if len(warningsInAllPaths) > 0 { - warnings = append(warnings, warningsInAllPaths...) - } + // remove duplicates path and sort the new array, so we can get predetermined result + uniqueSourcePaths := sets.New[string](sourcePaths...).UnsortedList() + orderedSourcePaths := append(uniqueSourcePaths, allPaths.UnsortedList()...) + sort.Strings(orderedSourcePaths) + warnings = append(warnings, checkVolumeMappingForOverlap(orderedSourcePaths, errorMessage)...) + allPaths.Insert(uniqueSourcePaths...) } } @@ -470,14 +463,14 @@ func extractPathsDownwardAPI(mapping []api.DownwardAPIVolumeFile) []string { } func checkVolumeMappingForOverlap(paths []string, warningMessage string) []string { + var warnings []string pathSeparator := string(os.PathSeparator) - warnings := make([]string, 0) uniquePaths := sets.New[string]() for _, path := range paths { normalizedPath := strings.TrimRight(path, pathSeparator) - if collision := checkForOverlap(uniquePaths, normalizedPath); collision != nil { - warnings = append(warnings, fmt.Sprintf("%s %q with %q", warningMessage, normalizedPath, *collision)) + if collision := checkForOverlap(uniquePaths, normalizedPath); collision != "" { + warnings = append(warnings, fmt.Sprintf("%s: %q with %q", warningMessage, normalizedPath, collision)) } uniquePaths.Insert(normalizedPath) } @@ -485,21 +478,21 @@ func checkVolumeMappingForOverlap(paths []string, warningMessage string) []strin return warnings } -func checkForOverlap(paths sets.Set[string], path string) *string { +func checkForOverlap(paths sets.Set[string], path string) string { pathSeparator := string(os.PathSeparator) - p := paths.UnsortedList() - sort.Strings(p) - for _, item := range p { + for item := range paths { switch { + case item == "" || path == "": + return "" case item == path: - return &item + return item case strings.HasPrefix(item+pathSeparator, path): - return &item + return item case strings.HasPrefix(path+pathSeparator, item): - return &item + return item } } - return nil + return "" } diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 9a70dc6261e..07aabb297f3 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -18,6 +18,7 @@ package pod import ( "context" + "strings" "testing" "k8s.io/apimachinery/pkg/api/resource" @@ -255,7 +256,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test\" with \"test\"", + `volume "Test" (ConfigMap "foo"): overlapping paths: "test" with "test"`, }, }, { @@ -277,7 +278,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test/app\" with \"test\"", + `volume "Test" (ConfigMap "foo"): overlapping paths: "test/app" with "test"`, }, }, { @@ -299,7 +300,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"Test\" (ConfigMap \"foo\"): overlapping path \"test\" with \"test/app\"", + `volume "Test" (ConfigMap "foo"): overlapping paths: "test" with "test/app"`, }, }, { @@ -321,7 +322,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"Test\" (Secret \"foo\"): overlapping path \"test\" with \"test\"", + `volume "Test" (Secret "foo"): overlapping paths: "test" with "test"`, }, }, { @@ -342,7 +343,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"Test\" (DownwardAPI): overlapping path \"test\" with \"test\"", + `volume "Test" (DownwardAPI): overlapping paths: "test" with "test"`, }, }, { @@ -354,15 +355,19 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, + { + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, }, - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }, }, }, }, @@ -370,11 +375,11 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, }, }, { - name: "overlapping paths in projected volume volume - service account dir and config file", + name: "overlapping paths in projected volume volume: service account dir and config file", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -382,15 +387,19 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, + { + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, }, - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test/file", - }}, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test/file", + }, + }, }, }, }, @@ -398,7 +407,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test/file\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/file" with "test"`, }, }, { @@ -410,15 +419,19 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test/file"}, + { + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test/file"}, + }, }, - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }, }, }, }, @@ -426,7 +439,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test/file\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/file" with "test"`, }, }, { @@ -438,16 +451,19 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, + { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, }, }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, }, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, }, }, }, @@ -455,7 +471,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, }, }, { @@ -467,14 +483,18 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + }, }, - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }, }, }, }, @@ -482,7 +502,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, }, }, { @@ -494,12 +514,16 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - Name: &testName, Path: "test", - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, + { + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + Name: &testName, Path: "test", + }, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }, }, }, }, @@ -507,7 +531,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, }, }, { @@ -519,12 +543,16 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - SignerName: &testName, Path: "test", - }}, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, + { + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test", + }, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }, }, }, }, @@ -532,7 +560,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, }, }, { @@ -544,15 +572,17 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, - Items: []api.KeyToPath{ - { - Key: "mykey", - Path: "test", + { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + { + Key: "mykey", + Path: "test", + }, }, }, - }}, + }, { ConfigMap: &api.ConfigMapProjection{ LocalObjectReference: api.LocalObjectReference{Name: "TestConfigMap"}, @@ -571,7 +601,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ConfigMap \"TestConfigMap\"): overlapping path \"test/test1\" with \"test\"", + `volume "foo" (Projected ConfigMap "TestConfigMap"): overlapping paths: "test/test1" with "test"`, }, }, { @@ -583,15 +613,17 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, - Items: []api.KeyToPath{ - { - Key: "mykey", - Path: "test", + { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + { + Key: "mykey", + Path: "test", + }, }, }, - }}, + }, { DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ @@ -606,11 +638,11 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected DownwardAPI): overlapping path \"test/test2\" with \"test\"", + `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test2" with "test"`, }, }, { - name: "overlapping paths in projected volume - downward api and downward api", + name: "overlapping paths in projected volume - downward api and cluster thrust bundle api", template: &api.PodTemplateSpec{Spec: api.PodSpec{ Volumes: []api.Volume{ { @@ -637,7 +669,7 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected ClusterTrustBundle \"Test\"): overlapping path \"test\" with \"test/test2\"", + `volume "foo" (Projected ClusterTrustBundle "Test"): overlapping paths: "test/test2" with "test"`, }, }, { @@ -649,24 +681,31 @@ func TestWarnings(t *testing.T) { VolumeSource: api.VolumeSource{ Projected: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - {ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - SignerName: &testName, Path: "test/test", - }}, - {DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - }, - }}, - {Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, + { + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test/test", }, }, + { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + }, + }, + }, + { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + }, + }, + }, + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, }, - {ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }}, }, }, }, @@ -674,11 +713,64 @@ func TestWarnings(t *testing.T) { }, }}, expected: []string{ - "volume \"foo\" (Projected DownwardAPI): overlapping path \"test\" with \"test/test\"", - "volume \"foo\" (Projected Secret \"Test\"): overlapping path \"test\" with \"test\"", - "volume \"foo\" (Projected Secret \"Test\"): overlapping path \"test\" with \"test/test\"", - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test/test\"", - "volume \"foo\" (Projected ServiceAccountToken): overlapping path \"test\" with \"test\"", + `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test" with "test"`, + `volume "foo" (Projected Secret "Test"): overlapping paths: "test" with "test"`, + `volume "foo" (Projected Secret "Test"): overlapping paths: "test/test" with "test"`, + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/test" with "test"`, + `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + }, + }, + { + name: "overlapping paths in projected volume - multiple sources", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test/test2", + }, + }, + { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + }, + }, + }, + }, + }, + }, + }, + }, + }}, + expected: []string{ + `volume "foo" (Projected DownwardAPI): overlapping paths: "test" with "test"`, + `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test2" with "test"`, + }, + }, + { + name: "empty sources in projected volume", + template: &api.PodTemplateSpec{Spec: api.PodSpec{ + Volumes: []api.Volume{ + { + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {}, + }, + }, + }, + }, + }, + }}, + expected: []string{ + `volume "foo" (Projected) has no sources provided`, }, }, { @@ -1513,12 +1605,16 @@ func TestWarnings(t *testing.T) { if tc.oldTemplate != nil { oldTemplate = tc.oldTemplate } - actual := sets.New[string](GetWarningsForPodTemplate(context.TODO(), nil, tc.template, oldTemplate)...) - expected := sets.New[string](tc.expected...) - for _, missing := range sets.List[string](expected.Difference(actual)) { + actual := GetWarningsForPodTemplate(context.TODO(), nil, tc.template, oldTemplate) + if len(actual) != len(tc.expected) { + t.Errorf("expected %d errors, got %d:\n%v", len(tc.expected), len(actual), strings.Join(actual, "\n")) + } + actualSet := sets.New(actual...) + expectedSet := sets.New(tc.expected...) + for _, missing := range sets.List(expectedSet.Difference(actualSet)) { t.Errorf("missing: %s", missing) } - for _, extra := range sets.List[string](actual.Difference(expected)) { + for _, extra := range sets.List(actualSet.Difference(expectedSet)) { t.Errorf("extra: %s", extra) } }) @@ -1531,12 +1627,16 @@ func TestWarnings(t *testing.T) { Spec: tc.template.Spec, } } - actual := sets.New[string](GetWarningsForPod(context.TODO(), pod, &api.Pod{})...) - expected := sets.New[string](tc.expected...) - for _, missing := range sets.List[string](expected.Difference(actual)) { + actual := GetWarningsForPod(context.TODO(), pod, &api.Pod{}) + if len(actual) != len(tc.expected) { + t.Errorf("expected %d errors, got %d:\n%v", len(tc.expected), len(actual), strings.Join(actual, "\n")) + } + actualSet := sets.New(actual...) + expectedSet := sets.New(tc.expected...) + for _, missing := range sets.List(expectedSet.Difference(actualSet)) { t.Errorf("missing: %s", missing) } - for _, extra := range sets.List[string](actual.Difference(expected)) { + for _, extra := range sets.List(actualSet.Difference(expectedSet)) { t.Errorf("extra: %s", extra) } }) @@ -1641,6 +1741,31 @@ func TestCheckForOverLap(t *testing.T) { path: "path/path1", expected: "path/path1", }, + "no match": { + checkPaths: []string{"path/path1"}, + path: "path2/path1", + expected: "", + }, + "empty checkPaths": { + checkPaths: []string{}, + path: "path2/path1", + expected: "", + }, + "empty string in checkPaths": { + checkPaths: []string{""}, + path: "path2/path1", + expected: "", + }, + "empty path": { + checkPaths: []string{"test"}, + path: "", + expected: "", + }, + "empty strings in checkPaths and path": { + checkPaths: []string{""}, + path: "", + expected: "", + }, "between file and dir": { checkPaths: []string{"path/path1"}, path: "path", @@ -1651,13 +1776,43 @@ func TestCheckForOverLap(t *testing.T) { path: "path/path1", expected: "path", }, + "multiple paths without overlap": { + checkPaths: []string{"path1/path", "path2/path", "path3/path"}, + path: "path4/path", + expected: "", + }, + "multiple paths with overlap": { + checkPaths: []string{"path1/path", "path2/path", "path3/path"}, + path: "path3/path3", + expected: "path3/path", + }, + "partial overlap": { + checkPaths: []string{"path1/path", "path2/path", "path3/path"}, + path: "path101/path3", + expected: "", + }, + "partial overlap in path": { + checkPaths: []string{"path101/path", "path2/path", "path3/path"}, + path: "path1/path3", + expected: "", + }, + "trailing slash in path": { + checkPaths: []string{"path1/path3"}, + path: "path1/path3/", + expected: "path1/path3", + }, + "trailing slash in checkPaths": { + checkPaths: []string{"path1/path3/"}, + path: "path1/path3", + expected: "path1/path3/", + }, } for name, tc := range testCase { t.Run(name, func(t *testing.T) { result := checkForOverlap(sets.New(tc.checkPaths...), tc.path) - if *result != tc.expected { - t.Errorf("expected %v, got %v", tc.expected, *result) + if result != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, result) } }) } From e1178c4cf87be2ff776e6f86f26546f01507c9bc Mon Sep 17 00:00:00 2001 From: Nikola Date: Mon, 22 Jul 2024 20:50:05 +0300 Subject: [PATCH 5/5] optimize checks for overlapping in projected --- pkg/api/pod/warnings.go | 133 ++++--- pkg/api/pod/warnings_test.go | 730 ++++++++++++++++------------------- 2 files changed, 418 insertions(+), 445 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 45ac5ec042a..a4c3dc711a9 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "os" - "sort" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -380,119 +379,149 @@ func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fi func warningsForOverlappingVirtualPaths(volumes []api.Volume) []string { var warnings []string + mkWarn := func(volName, volDesc, body string) string { + return fmt.Sprintf("volume %q (%s): overlapping paths: %s", volName, volDesc, body) + } + for _, v := range volumes { if v.ConfigMap != nil && v.ConfigMap.Items != nil { - warnings = append(warnings, checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items), fmt.Sprintf("volume %q (ConfigMap %q): overlapping paths", v.Name, v.ConfigMap.Name))...) + overlaps := checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items, "")) + for _, ol := range overlaps { + warnings = append(warnings, mkWarn(v.Name, fmt.Sprintf("ConfigMap %q", v.ConfigMap.Name), ol)) + } } if v.Secret != nil && v.Secret.Items != nil { - warnings = append(warnings, checkVolumeMappingForOverlap(extractPaths(v.Secret.Items), fmt.Sprintf("volume %q (Secret %q): overlapping paths", v.Name, v.Secret.SecretName))...) + overlaps := checkVolumeMappingForOverlap(extractPaths(v.Secret.Items, "")) + for _, ol := range overlaps { + warnings = append(warnings, mkWarn(v.Name, fmt.Sprintf("Secret %q", v.Secret.SecretName), ol)) + } } if v.DownwardAPI != nil && v.DownwardAPI.Items != nil { - warnings = append(warnings, checkVolumeMappingForOverlap(extractPathsDownwardAPI(v.DownwardAPI.Items), fmt.Sprintf("volume %q (DownwardAPI): overlapping paths", v.Name))...) + overlaps := checkVolumeMappingForOverlap(extractPathsDownwardAPI(v.DownwardAPI.Items, "")) + for _, ol := range overlaps { + warnings = append(warnings, mkWarn(v.Name, "DownwardAPI", ol)) + } } if v.Projected != nil { - var sourcePaths []string - var errorMessage string - allPaths := sets.New[string]() + var sourcePaths []pathAndSource + var allPaths []pathAndSource for _, source := range v.Projected.Sources { if source == (api.VolumeProjection{}) { warnings = append(warnings, fmt.Sprintf("volume %q (Projected) has no sources provided", v.Name)) continue } + switch { case source.ConfigMap != nil && source.ConfigMap.Items != nil: - sourcePaths = extractPaths(source.ConfigMap.Items) - errorMessage = fmt.Sprintf("volume %q (Projected ConfigMap %q): overlapping paths", v.Name, source.ConfigMap.Name) + sourcePaths = extractPaths(source.ConfigMap.Items, fmt.Sprintf("ConfigMap %q", source.ConfigMap.Name)) case source.Secret != nil && source.Secret.Items != nil: - sourcePaths = extractPaths(source.Secret.Items) - errorMessage = fmt.Sprintf("volume %q (Projected Secret %q): overlapping paths", v.Name, source.Secret.Name) + sourcePaths = extractPaths(source.Secret.Items, fmt.Sprintf("Secret %q", source.Secret.Name)) case source.DownwardAPI != nil && source.DownwardAPI.Items != nil: - sourcePaths = extractPathsDownwardAPI(source.DownwardAPI.Items) - errorMessage = fmt.Sprintf("volume %q (Projected DownwardAPI): overlapping paths", v.Name) + sourcePaths = extractPathsDownwardAPI(source.DownwardAPI.Items, "DownwardAPI") case source.ServiceAccountToken != nil: - sourcePaths = []string{source.ServiceAccountToken.Path} - errorMessage = fmt.Sprintf("volume %q (Projected ServiceAccountToken): overlapping paths", v.Name) + sourcePaths = []pathAndSource{{source.ServiceAccountToken.Path, "ServiceAccountToken"}} case source.ClusterTrustBundle != nil: - sourcePaths = []string{source.ClusterTrustBundle.Path} + name := "" if source.ClusterTrustBundle.Name != nil { - errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping paths", v.Name, *source.ClusterTrustBundle.Name) + name = *source.ClusterTrustBundle.Name } else { - errorMessage = fmt.Sprintf("volume %q (Projected ClusterTrustBundle %q): overlapping paths", v.Name, *source.ClusterTrustBundle.SignerName) + name = *source.ClusterTrustBundle.SignerName } + sourcePaths = []pathAndSource{{source.ClusterTrustBundle.Path, fmt.Sprintf("ClusterTrustBundle %q", name)}} } if len(sourcePaths) == 0 { continue } - warnings = append(warnings, checkVolumeMappingForOverlap(sourcePaths, errorMessage)...) - - // remove duplicates path and sort the new array, so we can get predetermined result - uniqueSourcePaths := sets.New[string](sourcePaths...).UnsortedList() - orderedSourcePaths := append(uniqueSourcePaths, allPaths.UnsortedList()...) - sort.Strings(orderedSourcePaths) - warnings = append(warnings, checkVolumeMappingForOverlap(orderedSourcePaths, errorMessage)...) - allPaths.Insert(uniqueSourcePaths...) + for _, ps := range sourcePaths { + ps.path = strings.TrimRight(ps.path, string(os.PathSeparator)) + if collisions := checkForOverlap(allPaths, ps); len(collisions) > 0 { + for _, c := range collisions { + warnings = append(warnings, mkWarn(v.Name, "Projected", fmt.Sprintf("%s with %s", ps.String(), c.String()))) + } + } + allPaths = append(allPaths, ps) + } } } - } return warnings } -func extractPaths(mapping []api.KeyToPath) []string { - result := make([]string, 0, len(mapping)) +// this lets us track a path and where it came from, for better errors +type pathAndSource struct { + path string + source string +} + +func (ps pathAndSource) String() string { + if ps.source != "" { + return fmt.Sprintf("%q (%s)", ps.path, ps.source) + } + return fmt.Sprintf("%q", ps.path) +} + +func extractPaths(mapping []api.KeyToPath, source string) []pathAndSource { + result := make([]pathAndSource, 0, len(mapping)) for _, v := range mapping { - result = append(result, v.Path) + result = append(result, pathAndSource{v.Path, source}) } return result } -func extractPathsDownwardAPI(mapping []api.DownwardAPIVolumeFile) []string { - result := make([]string, 0, len(mapping)) +func extractPathsDownwardAPI(mapping []api.DownwardAPIVolumeFile, source string) []pathAndSource { + result := make([]pathAndSource, 0, len(mapping)) for _, v := range mapping { - result = append(result, v.Path) + result = append(result, pathAndSource{v.Path, source}) } return result } -func checkVolumeMappingForOverlap(paths []string, warningMessage string) []string { +func checkVolumeMappingForOverlap(paths []pathAndSource) []string { + pathSeparator := string(os.PathSeparator) var warnings []string - pathSeparator := string(os.PathSeparator) - uniquePaths := sets.New[string]() + var allPaths []pathAndSource - for _, path := range paths { - normalizedPath := strings.TrimRight(path, pathSeparator) - if collision := checkForOverlap(uniquePaths, normalizedPath); collision != "" { - warnings = append(warnings, fmt.Sprintf("%s: %q with %q", warningMessage, normalizedPath, collision)) + for _, ps := range paths { + ps.path = strings.TrimRight(ps.path, pathSeparator) + if collisions := checkForOverlap(allPaths, ps); len(collisions) > 0 { + for _, c := range collisions { + warnings = append(warnings, fmt.Sprintf("%s with %s", ps.String(), c.String())) + } } - uniquePaths.Insert(normalizedPath) + allPaths = append(allPaths, ps) } return warnings } -func checkForOverlap(paths sets.Set[string], path string) string { +func checkForOverlap(haystack []pathAndSource, needle pathAndSource) []pathAndSource { pathSeparator := string(os.PathSeparator) - for item := range paths { + if needle.path == "" { + return nil + } + + var result []pathAndSource + for _, item := range haystack { switch { - case item == "" || path == "": - return "" - case item == path: - return item - case strings.HasPrefix(item+pathSeparator, path): - return item - case strings.HasPrefix(path+pathSeparator, item): - return item + case item.path == "": + continue + case item == needle: + result = append(result, item) + case strings.HasPrefix(item.path+pathSeparator, needle.path+pathSeparator): + result = append(result, item) + case strings.HasPrefix(needle.path+pathSeparator, item.path+pathSeparator): + result = append(result, item) } } - return "" + return result } diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 07aabb297f3..f3f0687131d 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -18,6 +18,7 @@ package pod import ( "context" + "reflect" "strings" "testing" @@ -240,20 +241,18 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in a configmap volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "Test", - VolumeSource: api.VolumeSource{ - ConfigMap: &api.ConfigMapVolumeSource{ - LocalObjectReference: api.LocalObjectReference{Name: "foo"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - {Key: "bar", Path: "test"}, - }, + Volumes: []api.Volume{{ + Name: "Test", + VolumeSource: api.VolumeSource{ + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test"}, }, }, }, - }, + }}, }}, expected: []string{ `volume "Test" (ConfigMap "foo"): overlapping paths: "test" with "test"`, @@ -262,20 +261,18 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in a configmap volume - try to mount dir path into a file", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "Test", - VolumeSource: api.VolumeSource{ - ConfigMap: &api.ConfigMapVolumeSource{ - LocalObjectReference: api.LocalObjectReference{Name: "foo"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - {Key: "bar", Path: "test/app"}, - }, + Volumes: []api.Volume{{ + Name: "Test", + VolumeSource: api.VolumeSource{ + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test/app"}, }, }, }, - }, + }}, }}, expected: []string{ `volume "Test" (ConfigMap "foo"): overlapping paths: "test/app" with "test"`, @@ -284,20 +281,18 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in a configmap volume - try to mount file into a dir path", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "Test", - VolumeSource: api.VolumeSource{ - ConfigMap: &api.ConfigMapVolumeSource{ - LocalObjectReference: api.LocalObjectReference{Name: "foo"}, - Items: []api.KeyToPath{ - {Key: "bar", Path: "test/app"}, - {Key: "foo", Path: "test"}, - }, + Volumes: []api.Volume{{ + Name: "Test", + VolumeSource: api.VolumeSource{ + ConfigMap: &api.ConfigMapVolumeSource{ + LocalObjectReference: api.LocalObjectReference{Name: "foo"}, + Items: []api.KeyToPath{ + {Key: "bar", Path: "test/app"}, + {Key: "foo", Path: "test"}, }, }, }, - }, + }}, }}, expected: []string{ `volume "Test" (ConfigMap "foo"): overlapping paths: "test" with "test/app"`, @@ -306,20 +301,18 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in a secret volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "Test", - VolumeSource: api.VolumeSource{ - Secret: &api.SecretVolumeSource{ - SecretName: "foo", - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - {Key: "bar", Path: "test"}, - }, + Volumes: []api.Volume{{ + Name: "Test", + VolumeSource: api.VolumeSource{ + Secret: &api.SecretVolumeSource{ + SecretName: "foo", + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, + {Key: "bar", Path: "test"}, }, }, }, - }, + }}, }}, expected: []string{ `volume "Test" (Secret "foo"): overlapping paths: "test" with "test"`, @@ -328,19 +321,17 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in a downward api volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "Test", - VolumeSource: api.VolumeSource{ - DownwardAPI: &api.DownwardAPIVolumeSource{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.labels"}, Path: "test"}, - }, + Volumes: []api.Volume{{ + Name: "Test", + VolumeSource: api.VolumeSource{ + DownwardAPI: &api.DownwardAPIVolumeSource{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.labels"}, Path: "test"}, }, }, }, - }, + }}, }}, expected: []string{ `volume "Test" (DownwardAPI): overlapping paths: "test" with "test"`, @@ -349,425 +340,357 @@ func TestWarnings(t *testing.T) { { name: "overlapping paths in projected volume - service account and config", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, }, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (ConfigMap "Test")`, }, }, { name: "overlapping paths in projected volume volume: service account dir and config file", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test/file", - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, }, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test/file", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/file" with "test"`, + `volume "foo" (Projected): overlapping paths: "test/file" (ServiceAccountToken) with "test" (ConfigMap "Test")`, }, }, { name: "overlapping paths in projected volume - service account file and config dir", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test/file"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test/file"}, }, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/file" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test/file" (ConfigMap "Test")`, }, }, { name: "overlapping paths in projected volume - service account and secret", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, }, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (Secret "Test")`, }, }, { name: "overlapping paths in projected volume - service account and downward api", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{{ + FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, + Path: "test", + }}, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (DownwardAPI)`, }, }, { name: "overlapping paths in projected volume - service account and cluster trust bundle", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - Name: &testName, Path: "test", - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + Name: &testName, Path: "test", }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (ClusterTrustBundle "Test")`, }, }, { name: "overlapping paths in projected volume - service account and cluster trust bundle with signer name", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - SignerName: &testName, Path: "test", - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test", }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (ClusterTrustBundle "Test")`, }, }, { name: "overlapping paths in projected volume - secret and config map", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, - Items: []api.KeyToPath{ - { - Key: "mykey", - Path: "test", - }, - }, - }, - }, - { - ConfigMap: &api.ConfigMapProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "TestConfigMap"}, - Items: []api.KeyToPath{ - { - Key: "mykey", - Path: "test/test1", - }, - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + {Key: "mykey", Path: "test"}, }, }, - }, + }, { + ConfigMap: &api.ConfigMapProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestConfigMap"}, + Items: []api.KeyToPath{ + {Key: "mykey", Path: "test/test1"}, + }, + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ConfigMap "TestConfigMap"): overlapping paths: "test/test1" with "test"`, + `volume "foo" (Projected): overlapping paths: "test/test1" (ConfigMap "TestConfigMap") with "test" (Secret "TestSecret")`, }, }, { name: "overlapping paths in projected volume - config map and downward api", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, - Items: []api.KeyToPath{ - { - Key: "mykey", - Path: "test", - }, - }, - }, - }, - { - DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test/test2"}, - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "TestSecret"}, + Items: []api.KeyToPath{ + {Key: "mykey", Path: "test"}, }, }, - }, + }, { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{{ + FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, + Path: "test/test2", + }}, + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test2" with "test"`, + `volume "foo" (Projected): overlapping paths: "test/test2" (DownwardAPI) with "test" (Secret "TestSecret")`, }, }, { name: "overlapping paths in projected volume - downward api and cluster thrust bundle api", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test/test2"}, - }, - }, - }, - { - ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - Name: &testName, Path: "test", - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{{ + FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, + Path: "test/test2", + }}, }, - }, + }, { + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + Name: &testName, Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected ClusterTrustBundle "Test"): overlapping paths: "test/test2" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (ClusterTrustBundle "Test") with "test/test2" (DownwardAPI)`, }, }, { name: "overlapping paths in projected volume - multiple sources", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ClusterTrustBundle: &api.ClusterTrustBundleProjection{ - SignerName: &testName, Path: "test/test", - }, - }, - { - DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - }, - }, - }, - { - Secret: &api.SecretProjection{ - LocalObjectReference: api.LocalObjectReference{Name: "Test"}, - Items: []api.KeyToPath{ - {Key: "foo", Path: "test"}, - }, - }, - }, - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test", - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ClusterTrustBundle: &api.ClusterTrustBundleProjection{ + SignerName: &testName, Path: "test/test"}, + }, { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{{ + FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, + Path: "test", + }}, + }, + }, { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{Name: "Test"}, + Items: []api.KeyToPath{ + {Key: "foo", Path: "test"}, }, }, - }, + }, { + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test", + }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test" with "test"`, - `volume "foo" (Projected Secret "Test"): overlapping paths: "test" with "test"`, - `volume "foo" (Projected Secret "Test"): overlapping paths: "test/test" with "test"`, - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test/test" with "test"`, - `volume "foo" (Projected ServiceAccountToken): overlapping paths: "test" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (DownwardAPI) with "test/test" (ClusterTrustBundle "Test")`, + `volume "foo" (Projected): overlapping paths: "test" (Secret "Test") with "test/test" (ClusterTrustBundle "Test")`, + `volume "foo" (Projected): overlapping paths: "test" (Secret "Test") with "test" (DownwardAPI)`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test/test" (ClusterTrustBundle "Test")`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (DownwardAPI)`, + `volume "foo" (Projected): overlapping paths: "test" (ServiceAccountToken) with "test" (Secret "Test")`, }, }, { - name: "overlapping paths in projected volume - multiple sources", + name: "overlapping paths in projected volume - ServiceAccount vs. DownwardAPI", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - { - ServiceAccountToken: &api.ServiceAccountTokenProjection{ - Path: "test/test2", - }, - }, - { - DownwardAPI: &api.DownwardAPIProjection{ - Items: []api.DownwardAPIVolumeFile{ - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, - }, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{{ + ServiceAccountToken: &api.ServiceAccountTokenProjection{ + Path: "test/test2", + }, + }, { + DownwardAPI: &api.DownwardAPIProjection{ + Items: []api.DownwardAPIVolumeFile{ + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, + {FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}, Path: "test"}, }, }, - }, + }}, }, }, - }, + }}, }}, expected: []string{ - `volume "foo" (Projected DownwardAPI): overlapping paths: "test" with "test"`, - `volume "foo" (Projected DownwardAPI): overlapping paths: "test/test2" with "test"`, + `volume "foo" (Projected): overlapping paths: "test" (DownwardAPI) with "test/test2" (ServiceAccountToken)`, + `volume "foo" (Projected): overlapping paths: "test" (DownwardAPI) with "test/test2" (ServiceAccountToken)`, + `volume "foo" (Projected): overlapping paths: "test" (DownwardAPI) with "test" (DownwardAPI)`, }, }, { name: "empty sources in projected volume", template: &api.PodTemplateSpec{Spec: api.PodSpec{ - Volumes: []api.Volume{ - { - Name: "foo", - VolumeSource: api.VolumeSource{ - Projected: &api.ProjectedVolumeSource{ - Sources: []api.VolumeProjection{ - {}, - }, + Volumes: []api.Volume{{ + Name: "foo", + VolumeSource: api.VolumeSource{ + Projected: &api.ProjectedVolumeSource{ + Sources: []api.VolumeProjection{ + {}, // one item, no fields set }, }, }, - }, + }}, }}, expected: []string{ `volume "foo" (Projected) has no sources provided`, @@ -1615,7 +1538,7 @@ func TestWarnings(t *testing.T) { t.Errorf("missing: %s", missing) } for _, extra := range sets.List(actualSet.Difference(expectedSet)) { - t.Errorf("extra: %s", extra) + t.Errorf("extra: %s", extra) } }) @@ -1637,7 +1560,7 @@ func TestWarnings(t *testing.T) { t.Errorf("missing: %s", missing) } for _, extra := range sets.List(actualSet.Difference(expectedSet)) { - t.Errorf("extra: %s", extra) + t.Errorf("extra: %s", extra) } }) } @@ -1732,86 +1655,107 @@ func TestTemplateOnlyWarnings(t *testing.T) { func TestCheckForOverLap(t *testing.T) { testCase := map[string]struct { - checkPaths []string - path string - expected string + checkPaths []pathAndSource + path pathAndSource + found bool + expected []pathAndSource }{ "exact match": { - checkPaths: []string{"path/path1"}, - path: "path/path1", - expected: "path/path1", + checkPaths: []pathAndSource{{"path/path1", "src1"}}, + path: pathAndSource{"path/path1", "src2"}, + found: true, + expected: []pathAndSource{{"path/path1", "src1"}}, }, "no match": { - checkPaths: []string{"path/path1"}, - path: "path2/path1", - expected: "", + checkPaths: []pathAndSource{{"path/path1", "src1"}}, + path: pathAndSource{"path2/path1", "src2"}, + found: false, }, "empty checkPaths": { - checkPaths: []string{}, - path: "path2/path1", - expected: "", + checkPaths: []pathAndSource{}, + path: pathAndSource{"path2/path1", "src2"}, + found: false, }, "empty string in checkPaths": { - checkPaths: []string{""}, - path: "path2/path1", - expected: "", + checkPaths: []pathAndSource{{"", "src1"}}, + path: pathAndSource{"path2/path1", "src2"}, + found: false, }, "empty path": { - checkPaths: []string{"test"}, - path: "", - expected: "", + checkPaths: []pathAndSource{{"test", "src1"}}, + path: pathAndSource{"", ""}, + found: false, }, "empty strings in checkPaths and path": { - checkPaths: []string{""}, - path: "", - expected: "", + checkPaths: []pathAndSource{{"", "src1"}}, + path: pathAndSource{"", ""}, + expected: []pathAndSource{{"", ""}}, + found: false, }, "between file and dir": { - checkPaths: []string{"path/path1"}, - path: "path", - expected: "path/path1", + checkPaths: []pathAndSource{{"path/path1", "src1"}}, + path: pathAndSource{"path", "src2"}, + found: true, + expected: []pathAndSource{{"path/path1", "src1"}}, }, "between dir and file": { - checkPaths: []string{"path"}, - path: "path/path1", - expected: "path", + checkPaths: []pathAndSource{{"path", "src1"}}, + path: pathAndSource{"path/path1", "src2"}, + found: true, + expected: []pathAndSource{{"path", "src1"}}, }, "multiple paths without overlap": { - checkPaths: []string{"path1/path", "path2/path", "path3/path"}, - path: "path4/path", - expected: "", + checkPaths: []pathAndSource{{"path1/path", "src1"}, {"path2/path", "src2"}, {"path3/path", "src3"}}, + path: pathAndSource{"path4/path", "src4"}, + found: false, }, - "multiple paths with overlap": { - checkPaths: []string{"path1/path", "path2/path", "path3/path"}, - path: "path3/path3", - expected: "path3/path", + "multiple paths with 1 overlap": { + checkPaths: []pathAndSource{{"path1/path", "src1"}, {"path2/path", "src2"}, {"path3/path", "src3"}}, + path: pathAndSource{"path3/path", "src4"}, + found: true, + expected: []pathAndSource{{"path3/path", "src3"}}, + }, + "multiple paths with multiple overlap": { + checkPaths: []pathAndSource{{"path/path1", "src1"}, {"path/path2", "src2"}, {"path/path3", "src3"}}, + path: pathAndSource{"path", "src4"}, + found: true, + expected: []pathAndSource{{"path/path1", "src1"}, {"path/path2", "src2"}, {"path/path3", "src3"}}, }, "partial overlap": { - checkPaths: []string{"path1/path", "path2/path", "path3/path"}, - path: "path101/path3", - expected: "", + checkPaths: []pathAndSource{{"path1/path", "src1"}, {"path2/path", "src2"}, {"path3/path", "src3"}}, + path: pathAndSource{"path101/path3", "src4"}, + found: false, }, "partial overlap in path": { - checkPaths: []string{"path101/path", "path2/path", "path3/path"}, - path: "path1/path3", - expected: "", + checkPaths: []pathAndSource{{"dir/path1", "src1"}, {"dir/path2", "src2"}, {"dir/path3", "src3"}}, + path: pathAndSource{"dir/path345", "src4"}, + found: false, }, "trailing slash in path": { - checkPaths: []string{"path1/path3"}, - path: "path1/path3/", - expected: "path1/path3", + checkPaths: []pathAndSource{{"path1/path3", "src1"}}, + path: pathAndSource{"path1/path3/", "src2"}, + found: true, + expected: []pathAndSource{{"path1/path3", "src1"}}, }, "trailing slash in checkPaths": { - checkPaths: []string{"path1/path3/"}, - path: "path1/path3", - expected: "path1/path3/", + checkPaths: []pathAndSource{{"path1/path3/", "src1"}}, + path: pathAndSource{"path1/path3", "src2"}, + found: true, + expected: []pathAndSource{{"path1/path3/", "src1"}}, }, } for name, tc := range testCase { t.Run(name, func(t *testing.T) { - result := checkForOverlap(sets.New(tc.checkPaths...), tc.path) - if result != tc.expected { + result := checkForOverlap(tc.checkPaths, tc.path) + found := len(result) > 0 + if found && !tc.found { + t.Errorf("unexpected match for %q: %q", tc.path, result) + } + if !found && tc.found { + t.Errorf("expected match for %q: %q", tc.path, tc.expected) + } + if tc.found && !reflect.DeepEqual(result, tc.expected) { t.Errorf("expected %q, got %q", tc.expected, result) } })