-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(docker): implement exclude paths functionality #4057
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
base: main
Are you sure you want to change the base?
Changes from all commits
54ee967
0e17df2
3e288ca
5f16bb9
ba66e58
d0f1a75
5c01d91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package docker | ||
|
||
import ( | ||
"regexp" | ||
"strings" | ||
"sync" | ||
"testing" | ||
|
@@ -166,3 +167,216 @@ func isHistoryChunk(t *testing.T, chunk *sources.Chunk) bool { | |
return metadata != nil && | ||
strings.HasPrefix(metadata.File, "image-metadata:history:") | ||
} | ||
|
||
func TestDockerExcludeExactPath(t *testing.T) { | ||
dockerConn := &sourcespb.Docker{ | ||
Credential: &sourcespb.Docker_Unauthenticated{ | ||
Unauthenticated: &credentialspb.Unauthenticated{}, | ||
}, | ||
Images: []string{"test-image"}, | ||
ExcludePaths: []string{"/var/log/test"}, | ||
} | ||
|
||
conn := &anypb.Any{} | ||
err := conn.MarshalFrom(dockerConn) | ||
assert.NoError(t, err) | ||
|
||
s := &Source{} | ||
err = s.Init(context.TODO(), "test source", 0, 0, false, conn, 1) | ||
assert.NoError(t, err) | ||
|
||
// Create test data | ||
testFiles := []struct { | ||
path string | ||
excluded bool | ||
}{ | ||
{"/var/log/test", true}, // Should be excluded (exact match) | ||
{"/var/log/test2", false}, // Should not be excluded (different path) | ||
{"/var/log/test/sub", false}, // Should not be excluded (subdirectory) | ||
{"/var/log/other", false}, // Should not be excluded (different path) | ||
} | ||
|
||
for _, tf := range testFiles { | ||
excluded := false | ||
for _, excludePath := range s.excludePaths { | ||
if tf.path == excludePath { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're only checking for exact matches here, which means that you're not actually exercising the match logic you've added to the source. This test is effectively adding a single item to a slice and then asserting that a few other items are not in the slice. You don't need a whole source implementation to test that! I think you can just remove this test entirely. |
||
excluded = true | ||
break | ||
} | ||
} | ||
assert.Equal(t, tf.excluded, excluded, "Unexpected exclusion result for path: %s", tf.path) | ||
} | ||
} | ||
|
||
func TestDockerExcludeWildcardPath(t *testing.T) { | ||
dockerConn := &sourcespb.Docker{ | ||
Credential: &sourcespb.Docker_Unauthenticated{ | ||
Unauthenticated: &credentialspb.Unauthenticated{}, | ||
}, | ||
Images: []string{"test-image"}, | ||
ExcludePaths: []string{"/var/log/test/*"}, | ||
} | ||
|
||
conn := &anypb.Any{} | ||
err := conn.MarshalFrom(dockerConn) | ||
assert.NoError(t, err) | ||
|
||
s := &Source{} | ||
err = s.Init(context.TODO(), "test source", 0, 0, false, conn, 1) | ||
assert.NoError(t, err) | ||
|
||
// Create test data | ||
testFiles := []struct { | ||
path string | ||
excluded bool | ||
}{ | ||
{"/var/log/test/file1", true}, // Should be excluded (direct child) | ||
{"/var/log/test/sub/file2", true}, // Should be excluded (nested child) | ||
{"/var/log/test", false}, // Should not be excluded (parent dir) | ||
{"/var/log/test2/file", false}, // Should not be excluded (different dir) | ||
{"/var/log/other/file", false}, // Should not be excluded (unrelated) | ||
} | ||
|
||
for _, tf := range testFiles { | ||
excluded := false | ||
for _, excludePath := range s.excludePaths { | ||
// Convert wildcard pattern to regex | ||
pattern := strings.ReplaceAll(excludePath, "*", ".*") | ||
pattern = "^" + pattern + "$" | ||
isMatch, err := regexp.MatchString(pattern, tf.path) | ||
assert.NoError(t, err) | ||
if isMatch { | ||
excluded = true | ||
break | ||
} | ||
} | ||
assert.Equal(t, tf.excluded, excluded, "Unexpected exclusion result for path: %s", tf.path) | ||
tannerjones4075 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
func TestShouldExclude(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
excludePaths []string | ||
testPath string | ||
want bool | ||
}{ | ||
{ | ||
name: "exact match should exclude", | ||
excludePaths: []string{"/var/log/test"}, | ||
testPath: "/var/log/test", | ||
want: true, | ||
}, | ||
{ | ||
name: "non-matching path should not exclude", | ||
excludePaths: []string{"/var/log/test"}, | ||
testPath: "/var/log/other", | ||
want: false, | ||
}, | ||
{ | ||
name: "wildcard should match children", | ||
excludePaths: []string{"/var/log/*"}, | ||
testPath: "/var/log/test", | ||
want: true, | ||
}, | ||
{ | ||
name: "wildcard should not match parent", | ||
excludePaths: []string{"/var/log/*/deep"}, | ||
testPath: "/var/log", | ||
want: false, | ||
}, | ||
{ | ||
name: "multiple patterns should work", | ||
excludePaths: []string{"/var/log/*", "/etc/nginx/*", "/tmp/test"}, | ||
testPath: "/etc/nginx/conf.d/default.conf", | ||
want: true, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
dockerConn := &sourcespb.Docker{ | ||
Credential: &sourcespb.Docker_Unauthenticated{ | ||
Unauthenticated: &credentialspb.Unauthenticated{}, | ||
}, | ||
ExcludePaths: tt.excludePaths, | ||
} | ||
|
||
conn := &anypb.Any{} | ||
err := conn.MarshalFrom(dockerConn) | ||
assert.NoError(t, err) | ||
|
||
s := &Source{} | ||
err = s.Init(context.TODO(), "test", 0, 0, false, conn, 1) | ||
assert.NoError(t, err) | ||
|
||
// Test if the path should be excluded | ||
excluded := false | ||
for _, path := range tt.excludePaths { | ||
if tt.testPath == path { | ||
excluded = true | ||
break | ||
} | ||
// Convert wildcard pattern to regex | ||
pattern := strings.ReplaceAll(path, "*", ".*") | ||
pattern = "^" + pattern + "$" | ||
isMatch, err := regexp.MatchString(pattern, tt.testPath) | ||
assert.NoError(t, err) | ||
if isMatch { | ||
excluded = true | ||
break | ||
} | ||
} | ||
assert.Equal(t, tt.want, excluded) | ||
Comment on lines
+313
to
+330
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love the idea of this new test! But notice how |
||
}) | ||
} | ||
} | ||
|
||
func TestDockerScanWithExclusions(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is great! |
||
dockerConn := &sourcespb.Docker{ | ||
Credential: &sourcespb.Docker_Unauthenticated{ | ||
Unauthenticated: &credentialspb.Unauthenticated{}, | ||
}, | ||
Images: []string{"trufflesecurity/secrets@sha256:864f6d41209462d8e37fc302ba1532656e265f7c361f11e29fed6ca1f4208e11"}, | ||
ExcludePaths: []string{"/aws"}, // This path exists in the test image | ||
} | ||
|
||
conn := &anypb.Any{} | ||
err := conn.MarshalFrom(dockerConn) | ||
assert.NoError(t, err) | ||
|
||
s := &Source{} | ||
err = s.Init(context.TODO(), "test source", 0, 0, false, conn, 1) | ||
assert.NoError(t, err) | ||
|
||
var wg sync.WaitGroup | ||
chunksChan := make(chan *sources.Chunk, 1) | ||
foundExcludedPath := false | ||
|
||
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
for chunk := range chunksChan { | ||
// Skip history chunks | ||
if isHistoryChunk(t, chunk) { | ||
continue | ||
} | ||
|
||
metadata := chunk.SourceMetadata.GetDocker() | ||
assert.NotNil(t, metadata) | ||
|
||
// Check if we found a chunk with the excluded path | ||
if metadata.File == "/aws" { | ||
foundExcludedPath = true | ||
} | ||
} | ||
}() | ||
|
||
err = s.Chunks(context.TODO(), chunksChan) | ||
assert.NoError(t, err) | ||
|
||
close(chunksChan) | ||
wg.Wait() | ||
|
||
assert.False(t, foundExcludedPath, "Found a chunk that should have been excluded") | ||
} |
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.
You might also consider adding the pattern being compiled to this error message; if the error is caused by something added during pattern generation, users are likely to be quite confused.