Skip to content

Commit 262fe68

Browse files
authored
fix: exclude untagged resources when tag inclusion filters are specified (#886) (#896)
This commit fixes an issue where resources with no tags were incorrectly included in results when tag-based inclusion filtering was configured. Previously, the ShouldInclude method would allow resources with nil or empty tags to pass through even when specific tag inclusion rules were defined. This behavior was inconsistent with user expectations that tag filters should only match resources that actually have the specified tags. Changes: - Modified ShouldIncludeBasedOnTag to return false for untagged resources when inclusion rules are present - Removed the nil/empty tag check in ShouldInclude that was bypassing tag-based filtering - Added comprehensive unit tests to verify the correct filtering behavior Fixes #886 Co-authored-by: James Kwon <[email protected]>
1 parent a6f8753 commit 262fe68

File tree

2 files changed

+115
-1
lines changed

2 files changed

+115
-1
lines changed

config/config.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,7 @@ func (r ResourceType) ShouldInclude(value ResourceValue) bool {
514514
return false
515515
} else if value.Time != nil && !r.ShouldIncludeBasedOnTime(*value.Time) {
516516
return false
517-
} else if value.Tags != nil && len(value.Tags) != 0 && !r.ShouldIncludeBasedOnTag(value.Tags) {
517+
} else if !r.ShouldIncludeBasedOnTag(value.Tags) {
518518
return false
519519
}
520520

config/config_tag_test.go

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
package config
2+
3+
import (
4+
"regexp"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
)
9+
10+
func TestShouldIncludeWithTagFilters(t *testing.T) {
11+
// Create a resource type with tag inclusion rules
12+
resourceTypeWithIncludeTags := ResourceType{
13+
IncludeRule: FilterRule{
14+
Tags: map[string]Expression{
15+
"env": {RE: *regexp.MustCompile("dev")},
16+
},
17+
},
18+
}
19+
20+
// Create a resource type with tag exclusion rules
21+
resourceTypeWithExcludeTags := ResourceType{
22+
ExcludeRule: FilterRule{
23+
Tags: map[string]Expression{
24+
"env": {RE: *regexp.MustCompile("prod")},
25+
},
26+
},
27+
}
28+
29+
// Create a resource type with both inclusion and exclusion rules
30+
resourceTypeWithBothRules := ResourceType{
31+
IncludeRule: FilterRule{
32+
Tags: map[string]Expression{
33+
"env": {RE: *regexp.MustCompile("dev")},
34+
},
35+
},
36+
ExcludeRule: FilterRule{
37+
Tags: map[string]Expression{
38+
"team": {RE: *regexp.MustCompile("finance")},
39+
},
40+
},
41+
}
42+
43+
tests := []struct {
44+
name string
45+
resourceType ResourceType
46+
resourceTags map[string]string
47+
expected bool
48+
}{
49+
{
50+
name: "Resource with matching include tag should be included",
51+
resourceType: resourceTypeWithIncludeTags,
52+
resourceTags: map[string]string{"env": "dev", "team": "engineering"},
53+
expected: true,
54+
},
55+
{
56+
name: "Resource with non-matching include tag should be excluded",
57+
resourceType: resourceTypeWithIncludeTags,
58+
resourceTags: map[string]string{"env": "prod", "team": "engineering"},
59+
expected: false,
60+
},
61+
{
62+
name: "Resource with no tags should be excluded when include tag rule exists",
63+
resourceType: resourceTypeWithIncludeTags,
64+
resourceTags: nil,
65+
expected: false,
66+
},
67+
{
68+
name: "Resource with empty tags should be excluded when include tag rule exists",
69+
resourceType: resourceTypeWithIncludeTags,
70+
resourceTags: map[string]string{},
71+
expected: false,
72+
},
73+
{
74+
name: "Resource with matching exclude tag should be excluded",
75+
resourceType: resourceTypeWithExcludeTags,
76+
resourceTags: map[string]string{"env": "prod", "team": "engineering"},
77+
expected: false,
78+
},
79+
{
80+
name: "Resource with non-matching exclude tag should be included",
81+
resourceType: resourceTypeWithExcludeTags,
82+
resourceTags: map[string]string{"env": "dev", "team": "engineering"},
83+
expected: true,
84+
},
85+
{
86+
name: "Resource with no tags should be included when only exclude tag rule exists",
87+
resourceType: resourceTypeWithExcludeTags,
88+
resourceTags: nil,
89+
expected: true,
90+
},
91+
{
92+
name: "Resource with matching include tag but also matching exclude tag should be excluded",
93+
resourceType: resourceTypeWithBothRules,
94+
resourceTags: map[string]string{"env": "dev", "team": "finance"},
95+
expected: false,
96+
},
97+
{
98+
name: "Resource with matching include tag and non-matching exclude tag should be included",
99+
resourceType: resourceTypeWithBothRules,
100+
resourceTags: map[string]string{"env": "dev", "team": "engineering"},
101+
expected: true,
102+
},
103+
}
104+
105+
for _, tc := range tests {
106+
t.Run(tc.name, func(t *testing.T) {
107+
resourceValue := ResourceValue{
108+
Tags: tc.resourceTags,
109+
}
110+
result := tc.resourceType.ShouldInclude(resourceValue)
111+
assert.Equal(t, tc.expected, result)
112+
})
113+
}
114+
}

0 commit comments

Comments
 (0)