-
Notifications
You must be signed in to change notification settings - Fork 40.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add warnings for cases one of projected volume types get overwritten by service account token #121968
Conversation
Hi @Peac36. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@smarterclayton, @thockin - can you review the PR? |
/sig auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have a Condition or something, set by Kubelet when actually setting up the volumes? That is the only time we REALLY know if there is a collision, and even that can change dynamically.
pkg/api/pod/warnings.go
Outdated
if v.Projected != nil { | ||
for _, k := range v.Projected.Sources { | ||
if k.ServiceAccountToken != nil { | ||
items.Insert(k.ServiceAccountToken.Path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClusterTrustBundle also has a path
- should that be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is handled when the pod is validated
pkg/api/pod/warnings.go
Outdated
if k.ConfigMap != nil { | ||
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this accumulate all paths and just complain if ANY of them overlap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have something similar on the validation step but while validating we don't check if the service account token path is unique. Or do you want to just simplify the logic here?
Nevermind, I got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but my understanding of the issue here is that we don't check for the uniqueness of the paths in the projected volume service account token with other items from projected volumes. So I believe the issue is scoped to the projected volumes only. That's why I'm checking only projected volumes. Let me know if I'm missing something.
pkg/api/pod/warnings.go
Outdated
if k.ConfigMap != nil { | ||
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't help if the Items field is not specified, right? It is strictly opional and defaults to "all keys in the map"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
/triage accepted |
/retest |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@thockin can you give it another try? |
My last comment on #121414 still applies. This will AT BEST be partial coverage. Is this really the right way to handle the problem, or is there a deeper place where we can catch ALL such errors? On one hand, some warning is better than none. On the other hand, it provides a false sense of security - you could STILL have conflicts, I think. Maybe, before we approve this, we should do a more careful audit of all the volumes that could be involved.
Are there others? We have several aspects that factor:
I can use these in a pod:
and define them as:
Then:
As expected. Now can I force them to collide?
Naively, I would expect /mnt to have foo, bar, and qux files.
If you know how kubelet works, this is not too surprising - it bind-mounts directories, which does not merge, but over-writes. It's not obvious which one SHOULD win. Perhaps we should define that (@dchen1107 @msau42)? There was no API warning that I have set up collisions. We should fix that (this PR). There are no events or other status that would indicate to me that something went wrong. We should probably fix that (@dchen1107, @msau42). Also, I can use The take-away here is that my concerns about CM and Secrets also colliding, not just Projected, is correct but maybe not as dire as I thought, since you have to EXPLICITLY use subdirs in This doesn't handle something like a local volume or a hostPath with nested mounts. At some point you need to let the user own their own decisions. So: option 1) Do nothing here, let users do things they want. After typing this up, I think option 2 is a fair tradeoff, which this PR starts to address (but I think it should be for ALL paths, not specific to SA Token). Shouldn't we also do warnings for collisions within Secret and ConfigMap volumes in this same PR (different commits)? |
/remove-lifecycle stale |
20184dd
to
aae93f2
Compare
pkg/api/pod/warnings.go
Outdated
normalizedPath := strings.TrimRight(path, pathSeparator) | ||
if checkForOverlap(uniquePaths, normalizedPath) { | ||
args := append(warningMessageArgument, normalizedPath) | ||
warnings = append(warnings, fmt.Sprintf(warningMessage, args...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any time I see a printf with a non-const message, I see an opportunity for screwup.
Simpler:
Change this to take warningMessage string
and no arguments. Let callers do:
w := checkVolumeMappingForOverlap(extractPaths(v.ConfigMap.Items),
fmt.Sprintf("volume %q (ConfigMap %q): overlapping paths", v.Name, v.ConfigMap.Name))
This function just does append... fmt.Sprintf("%s: %s", warningMessage, normalizedPath)
.
Any reason that can't work?
While we are at it, it might be nice to print BOTH paths.
normalizedPath := strings.TrimRight(path, pathSeparator)
if collision := checkForOverlap(uniquePaths, normalizedPath); collision != "" {
warnings = append(warnings, fmt.Sprintf("%s: %s, %s", warningMessage, collision, normalizedPath))
}
uniquePaths.Insert(normalizedPath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason that can't work?
Nope. Implemented.
While we are at it, it might be nice to print BOTH paths.
Yeah, like the idea of having both paths in the error message - implemented.
pkg/api/pod/warnings.go
Outdated
|
||
for _, item := range source.ConfigMap.Items { | ||
normalizedPath = strings.TrimRight(item.Path, pathSeparator) | ||
if checkForOverlap(pathSets, normalizedPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this doing exactly what checkVolumeMappingForOverlap does, but bigger? Why call both here?
Edit: I see we don't ever add to pathSets
- why? Given that this is a single Projected volume, we should be able to detect collision of a ConfigMap vs a Secret, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I relied on validation to cover all duplicated paths except the ones that include service account token which was the initial idea of the PR and the idea behind this code. But then I realized that the validation logic covers only exact matches and skips other cases such as mounting dir to an already mounted file, etc.
So I've refactored the whole function to work with all possible projected sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there
pkg/api/pod/warnings.go
Outdated
} | ||
|
||
allPaths = append(allPaths, sourcePaths...) | ||
warningsInAllPaths := checkVolumeMappingForOverlap(allPaths, errorMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still confused - you just appended sourcePaths
to allPaths
which should trigger the same warnings? I feel like I am missing something -- what's the point of the first check above (within sourcePaths) if we are just going to check against allPaths?
Edit: Looking at the test, it bounces thru a set, which hides the double errors! We should fix the test to be smarter - you can do it in here or we can do it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to convince myself it was not hiding others. Here's a diff you can patch in and I think it will show the error here:
diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go
index 6405977b7cb..c813a78ee4c 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"
@@ -1067,12 +1068,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)
}
})
@@ -1085,12 +1090,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)
}
})
I took this apart in excruciating detail, and I realized what you were up to. The problem is that you want to print useful errors, but the sets and merged lists are losing information. I changed it to track paths and sources TOGETHER, which is a little more verbose but I think the errors are better, and there's no O(n^2) sort-in-a-loop stuff. The errors are not sorted but are a deterministic order, specifically the order we check the sources, and do not duplicate. E.g. if "test" overlaps with "test/foo", you get one error for I also fixed up a testcase which showed a prefix bug (yay testcases!). My diff is here: 74e87d8 Please take a look if it makes sense, you can steal it for your PR. I think I am OK to merge it with these changes. or inline: commit 74e87d8e23a50d97710a27b0fffcc7b9e1c5b8d2
Good "git" signature for thockin@google.com with ED25519 key SHA256:M2+rk0pn34LtCBIVneq+UkpS+MLUTLylIUjHPq6OGSE
Author: Tim Hockin <thockin@google.com>
Date: Sun Jul 14 12:47:18 2024 -0700
Rework to track path-and-source together
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)
}
}) |
Yes, my idea regarding the projected volumes was to compare the current path against all unique paths already checked so we don't get duplicate error messages for cases where duplicate paths are in a single projected volume and across several projected volumes at the same time. This is where we are losing information, I guess. Btw I like your solution, the new error messages are much better than mine. |
Let me know when this is ready for a re-review |
@thockin - it's ready for re-review. The patch has been applied and all other issues have been resolved. |
@thockin - let me know if something else needs to be addressed. |
@thockin - just a reminder that this can be reviewed again. |
Thanks! /lgtm |
LGTM label has been added. Git tree hash: f815d160257cc89f5e569d4ff397a19f8a686745
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Peac36, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/milestone v1.32 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #121414
Special notes for your reviewer:
Add warnings whenever there are overlaps in the virtual mounting paths for the following volumes:
Example
This would produce a warning message because
path/path2
is contained inpath
.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: