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.

Sign up for GitHub

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

Fix handling of 'has' selectors #473

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Fix handling of 'has' selectors #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac0744f
Add tests to surface issue 467
antoineco Oct 11, 2021
99c2c06
Fall back to empty map if object has no labels
antoineco Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions pkg/resolve/selector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ func MatchesSelector(doc *yaml.Node, selector labels.Selector) (bool, error) {
if err != nil {
return false, err
}
if kind == "" {
return false, nil
}
Comment on lines +41 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only case where docKind() returns an empty kind string without error is when the node is a !!null node.

I'm not sure under what circumstances such node is passed to MatchesSelector(), but I had to short-circuit here because objMatchesSelector() doesn't understand what a !!null node is.

I'm happy to remove this check on kind and add a check for !!null to objMatchesSelector() instead, in case someone feels strongly about it.


if kind == "List" {
return listMatchesSelector(doc, selector)
Expand Down Expand Up @@ -96,11 +99,12 @@ func objMatchesSelector(doc *yaml.Node, selector labels.Selector) bool {

node, ok := it()

if ok && selector.Matches(labelsNode{node}) {
return true
// Object has no metadata.labels, verify matching against an empty set.
if !ok {
node = emptyMapNode
}

return false
return selector.Matches(labelsNode{node})
}

func listMatchesSelector(doc *yaml.Node, selector labels.Selector) (bool, error) {
Expand Down Expand Up @@ -134,6 +138,11 @@ func listMatchesSelector(doc *yaml.Node, selector labels.Selector) (bool, error)
return len(matches) != 0, nil
}

var emptyMapNode = &yaml.Node{
Kind: yaml.MappingNode,
Tag: "!!map",
}

type labelsNode struct {
*yaml.Node
}
Expand Down
63 changes: 57 additions & 6 deletions pkg/resolve/selector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ var (
webSelector = selector(`app=web`)
notWebSelector = selector(`app!=web`)
nopSelector = selector(`foo!=bark`)

hasSelector = selector(`app`)
notHasSelector = selector(`!app`)
)

const (
Expand All @@ -44,6 +47,11 @@ metadata:
# comments should be preserved
app: db
name: rss-db
`
podNoLabel = `apiVersion: v1
kind: Pod
metadata:
name: rss-site
`
podList = `apiVersion: v1
kind: List
Expand Down Expand Up @@ -89,6 +97,21 @@ items:
labels:
app: db
name: rss-db
`
podListNoLabel = `apiVersion: v1
kind: List
metadata:
resourceVersion: ""
selfLink: ""
items:
- apiVersion: v1
kind: Pod
metadata:
name: rss-site
- apiVersion: v1
kind: Pod
metadata:
name: rss-db
`
)

Expand Down Expand Up @@ -116,6 +139,28 @@ func TestMatchesSelector(t *testing.T) {
selector: nopSelector,
output: dbPod,
matches: true,
}, {
desc: "single object with has selector",
input: webPod,
selector: hasSelector,
output: webPod,
matches: true,
}, {
desc: "single object with not-has selector",
input: webPod,
selector: notHasSelector,
matches: false,
}, {
desc: "single non-labeled object with has selector",
input: podNoLabel,
selector: hasSelector,
matches: false,
}, {
desc: "single non-labeled object with not-has selector",
input: podNoLabel,
selector: notHasSelector,
output: podNoLabel,
matches: true,
}, {
desc: "selector matching elements of list object",
input: podList,
Expand All @@ -128,22 +173,28 @@ func TestMatchesSelector(t *testing.T) {
selector: notWebSelector,
output: dbPodList,
matches: true,
}, {
desc: "has selector matching no non-labeled element of list object",
input: podListNoLabel,
selector: hasSelector,
matches: false,
}, {
desc: "not-has selector matching all non-labeled elements of list object",
input: podListNoLabel,
selector: notHasSelector,
output: podListNoLabel,
matches: true,
}, {
desc: "selector matching all elements of list object",
input: podList,
selector: labels.Everything(),
output: podList,
matches: true,
}, {
desc: "selector matching no elements of list object",
desc: "selector matching no element of list object",
input: podList,
selector: labels.Nothing(),
matches: false,
}, {
desc: "null node",
input: "!!null",
selector: labels.Everything(),
matches: false,
Comment on lines -142 to -146
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment for the reason for this removal.

}}

for _, test := range tests {
Expand Down