Skip to content
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

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

Peac36
Copy link
Contributor

@Peac36 Peac36 commented Nov 20, 2023

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:

  • ConfigMap
  • Secret
  • DownwardAPI
  • Projected

Example

name: myconfig
items:
   - key: key1
     path: path
   - key: key2
     path: path/path2

This would produce a warning message because path/path2 is contained in path.

Does this PR introduce a user-facing change?

* Add warnings for overlap paths in ConfigMap, Secret, DownwardAPI, Projected
* Add warning for cases when ProjectedVolume with sources is provided. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 20, 2023
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 20, 2023
@Peac36
Copy link
Contributor Author

Peac36 commented Nov 28, 2023

@smarterclayton, @thockin - can you review the PR?

@dims
Copy link
Member

dims commented Jan 2, 2024

/sig auth

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 2, 2024
Copy link
Member

@thockin thockin left a 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.

if v.Projected != nil {
for _, k := range v.Projected.Sources {
if k.ServiceAccountToken != nil {
items.Insert(k.ServiceAccountToken.Path)
Copy link
Member

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?

Copy link
Contributor Author

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

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))
Copy link
Member

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?

Copy link
Contributor Author

@Peac36 Peac36 Jan 7, 2024

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.

Copy link
Contributor Author

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.

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))
Copy link
Member

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@stlaz
Copy link
Member

stlaz commented Jan 29, 2024

/triage accepted
/ok-to-test

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 29, 2024
@Peac36
Copy link
Contributor Author

Peac36 commented Jan 30, 2024

/retest

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 29, 2024
@Peac36
Copy link
Contributor Author

Peac36 commented May 1, 2024

@thockin can you give it another try?

@thockin
Copy link
Member

thockin commented May 1, 2024

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.

  1. configMap
  2. secret
  3. projected

Are there others?

We have several aspects that factor:

  1. Pod.spec.containers[].volumeMounts[] must have unique mount paths (can't mount 2 volumes on the same path). Let's assume we don't have to worry about tricks with symlinks via hostPath (but we may want to revisit that).

  2. ConfigMap and Secret volumes will use the keys of the ConfigMap/Secret by default (if items is not specified). Those keys may not contain / nor may they start with "..", so they are locked to a single directory.

  3. I can use items to get create subdirs. Example:

$ k get cm foo -o yaml
apiVersion: v1
data:
  bar: bar-from-cm-foo
  foo: foo-from-cm-foo
kind: ConfigMap
metadata:
  creationTimestamp: "2024-05-01T18:23:37Z"
  name: foo
  namespace: default
  resourceVersion: "7317002"
  uid: 22e5347b-9f39-476e-8afd-12f48b5253d9

$ k get cm bar -o yaml
apiVersion: v1
data:
  bar: bar-from-cm-bar
  qux: qux-from-cm-bar
kind: ConfigMap
metadata:
  creationTimestamp: "2024-05-01T18:24:28Z"
  name: bar
  namespace: default
  resourceVersion: "7317041"
  uid: ce3c987b-c12b-4996-a066-633ad1c9ad1c

$ k get secret qux -o yaml
apiVersion: v1
data:
  qux: cXV4LWZyb20tc2VjcmV0LXF1eA==
kind: Secret
metadata:
  creationTimestamp: "2024-05-01T18:41:31Z"
  name: qux
  namespace: default
  resourceVersion: "7317114"
  uid: 34950e9f-7ee7-4569-83c7-a633b72feb11
type: Opaque

I can use these in a pod:

       volumeMounts:
        - mountPath: /mnt
          name: foo 
        - mountPath: /mnt/sub
          name: bar 
        - mountPath: /mnt/sub/sub
          name: qux 

and define them as:

      volumes:
      - name: foo
        configMap:
          name: foo
          items:
          - key: foo
            path: foo
          - key: bar
            path: bar
      - name: bar
        configMap:
          name: bar
          items:
          - key: bar
            path: bar
          - key: qux
            path: qux
      - name: qux
        secret:
          secretName: qux
          items:
          - key: qux
            path: qux

Then:

$ k exec -ti hostnames-5f8dfd7564-t5l7g -- sh
/ # ls -l /mnt
total 4
lrwxrwxrwx    1 root     root            10 May  1 18:53 bar -> ..data/bar
lrwxrwxrwx    1 root     root            10 May  1 18:53 foo -> ..data/foo
drwxrwxrwx    4 root     root          4096 May  1 18:53 sub

/ # ls -l /mnt/sub
total 0
lrwxrwxrwx    1 root     root            10 May  1 18:53 bar -> ..data/bar
lrwxrwxrwx    1 root     root            10 May  1 18:53 qux -> ..data/qux
drwxrwxrwt    3 root     root           100 May  1 18:53 sub

/ # ls -l /mnt/sub/sub
total 0
lrwxrwxrwx    1 root     root            10 May  1 18:53 qux -> ..data/qux

As expected. Now can I force them to collide?

      volumes:
      - name: foo 
        configMap:
          name: foo 
          items:
          - key: foo 
            path: sub/sub/foo
          - key: bar 
            path: sub/sub/bar
      - name: bar 
        configMap:
          name: bar 
          items:
          - key: bar 
            path: sub/bar
          - key: qux 
            path: sub/qux
      - name: qux 
        secret:
          secretName: qux 
          items:
          - key: qux 
            path: qux 

Naively, I would expect /mnt to have foo, bar, and qux files.

$ k exec -ti hostnames-76749566fb-b27x8 -- sh
/ # ls -l /mnt
total 4
drwxrwxrwx    4 root     root          4096 May  1 18:58 sub

/ # ls -l /mnt/sub
total 0
drwxrwxrwt    3 root     root           100 May  1 18:58 sub

/ # ls -l /mnt/sub/sub
total 0
lrwxrwxrwx    1 root     root            10 May  1 18:58 qux -> ..data/qux

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 items to map multiple keys to the same path. We should warn on that.

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 items. So if we are going to warn about collisions, should we scan each container for configMap, secret, and projected volumeMounts which collide (note: different containers could mount them differently in the same pod!) and warn for them all?

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.
option 2) Warn on collisions, but only within a single volume definition. If users overlap volume mounts, that's on them.
option 3) Warn on collisions across all "virtual" volumes, knowing that users can still do dumb things with other volume types.

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)?

@Peac36
Copy link
Contributor Author

Peac36 commented May 10, 2024

/remove-lifecycle stale

@Peac36 Peac36 force-pushed the fix/121414 branch 2 times, most recently from 20184dd to aae93f2 Compare July 5, 2024 15:06
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
normalizedPath := strings.TrimRight(path, pathSeparator)
if checkForOverlap(uniquePaths, normalizedPath) {
args := append(warningMessageArgument, normalizedPath)
warnings = append(warnings, fmt.Sprintf(warningMessage, args...))
Copy link
Member

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)

Copy link
Contributor Author

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 Show resolved Hide resolved

for _, item := range source.ConfigMap.Items {
normalizedPath = strings.TrimRight(item.Path, pathSeparator)
if checkForOverlap(pathSets, normalizedPath) {
Copy link
Member

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?

Copy link
Contributor Author

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.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2024
Copy link
Member

@thockin thockin left a 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 Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings_test.go Show resolved Hide resolved
}

allPaths = append(allPaths, sourcePaths...)
warningsInAllPaths := checkVolumeMappingForOverlap(allPaths, errorMessage)
Copy link
Member

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.

Copy link
Member

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)
 			}
 		})

pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings_test.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings_test.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings_test.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings_test.go Outdated Show resolved Hide resolved
pkg/api/pod/warnings.go Outdated Show resolved Hide resolved
@thockin
Copy link
Member

thockin commented Jul 14, 2024

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 "test" with "test/foo" and not the reciprocal.

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)
 			}
 		})

@Peac36
Copy link
Contributor Author

Peac36 commented Jul 24, 2024

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.

@thockin
Copy link
Member

thockin commented Jul 30, 2024

Let me know when this is ready for a re-review

@thockin thockin self-assigned this Jul 30, 2024
@Peac36
Copy link
Contributor Author

Peac36 commented Aug 4, 2024

@thockin - it's ready for re-review. The patch has been applied and all other issues have been resolved.

@Peac36
Copy link
Contributor Author

Peac36 commented Sep 6, 2024

@thockin - let me know if something else needs to be addressed.

@Peac36 Peac36 requested a review from thockin September 15, 2024 15:46
@Peac36
Copy link
Contributor Author

Peac36 commented Oct 15, 2024

@thockin - just a reminder that this can be reviewed again.

@thockin
Copy link
Member

thockin commented Nov 8, 2024

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f815d160257cc89f5e569d4ff397a19f8a686745

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024
@pacoxu
Copy link
Member

pacoxu commented Nov 8, 2024

/milestone v1.32
approved before code freeze

@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Nov 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit a2a32fc into kubernetes:master Nov 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Service account token projected volume validation should require the token path to be unique
7 participants