Skip to content

Commit 210c475

Browse files
authored
fix: Correct multiple labels filtering for get commadn (#59)
## Motivation Our backend only interprets the first query parameter value it receives. Since we've been setting `url.Values` labels query parameter key to a list of parsed values, this caused our backend to only interpret the first value. With example Projects: ```yaml - apiVersion: n9/v1alpha kind: Project metadata: displayName: azure-agent labels: env: - dev - dev-demo-1 team: - bees name: azure-agent spec: description: azure-agent - apiVersion: n9/v1alpha kind: Project metadata: labels: team: - bees name: project-two spec: description: "" ``` When running: ```sh sloctl get project -l env=dev,team=bees sloctl get project -l team=bees,env=dev ``` We got: - for the first command, both Projects - for the second one, only one Project, `azure-agent` **What should've happened?** Both commands should only return `azure-agent`, as this should've been interpreted as AND condition between labels. As a side effect, OR conditions have been broken too (same key, multiple labels), so these should work now too. ## Testing - Extended e2e tests coverage. ## Release Notes Fixed labels filtering for `sloctl get` command which was causing incorrect results to be returned by our backend. Issue affects every sloctl version starting from and including v0.0.95.
1 parent 59fbe90 commit 210c475

File tree

4 files changed

+152
-94
lines changed

4 files changed

+152
-94
lines changed

internal/get.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -358,9 +358,9 @@ func (g *GetCmd) getObjects(ctx context.Context, args []string, kind manifest.Ki
358358
return nil, errors.New("'sloctl get alerts' does not support Project filtering," +
359359
" explicitly pass '-A' flag to fetch all Alerts.")
360360
}
361-
query := url.Values{
362-
objectsV1.QueryKeyName: args,
363-
objectsV1.QueryKeyLabels: parseFilterLabel(g.labels),
361+
query := url.Values{objectsV1.QueryKeyName: args}
362+
if len(g.labels) > 0 {
363+
query.Set(objectsV1.QueryKeyLabels, parseFilterLabel(g.labels))
364364
}
365365
header := http.Header{sdk.HeaderProject: []string{g.client.Config.Project}}
366366
objects, err := g.client.Objects().V1().Get(ctx, kind, header, query)
@@ -379,7 +379,7 @@ func (g *GetCmd) getObjects(ctx context.Context, args []string, kind manifest.Ki
379379
return objects, nil
380380
}
381381

382-
func parseFilterLabel(filterLabels []string) []string {
382+
func parseFilterLabel(filterLabels []string) string {
383383
labels := make(v1alpha.Labels)
384384
for _, filterLabel := range filterLabels {
385385
filteredLabels := strings.Split(filterLabel, ",")
@@ -404,7 +404,7 @@ func parseFilterLabel(filterLabels []string) []string {
404404
strLabels = append(strLabels, key)
405405
}
406406
}
407-
return strLabels
407+
return strings.Join(strLabels, ",")
408408
}
409409

410410
func (g *GetCmd) printObjects(objects interface{}) error {

test/get.bats

+61-15
Original file line numberDiff line numberDiff line change
@@ -84,19 +84,65 @@ setup() {
8484
}
8585

8686
@test "agent with keys" {
87-
for flag in -k --with-keys; do
88-
run_sloctl get agent -p "death-star" "$flag"
89-
assert_success
90-
# Assert length of client_id and regex of client_secret, as the latter may vary.
91-
client_id="$(yq -r .[].metadata.client_id <<<"$output")"
92-
client_secret="$(yq -r .[].metadata.client_secret <<<"$output")"
93-
assert_equal "${#client_id}" 20
94-
assert_regex "${#client_secret}" "[a-zA-Z0-9_-]+"
95-
# Finally make sure the whole Agent definition is being presented.
87+
for flag in -k --with-keys; do
88+
run_sloctl get agent -p "death-star" "$flag"
89+
assert_success
90+
# Assert length of client_id and regex of client_secret, as the latter may vary.
91+
client_id="$(yq -r .[].metadata.client_id <<<"$output")"
92+
client_secret="$(yq -r .[].metadata.client_secret <<<"$output")"
93+
assert_equal "${#client_id}" 20
94+
assert_regex "${#client_secret}" "[a-zA-Z0-9_-]+"
95+
# Finally make sure the whole Agent definition is being presented.
9696
verify_get_success "$output" "$(read_files "${TEST_INPUTS}/agent.yaml")"
97-
done
97+
done
98+
}
99+
100+
@test "projects, multiple names" {
101+
run_sloctl get project death-star hoth-base
102+
verify_get_success "$output" "$(read_files "${TEST_INPUTS}/projects.yaml")"
98103
}
99104

105+
@test "projects, labels filtering, OR conditions" {
106+
want=$(read_files "${TEST_INPUTS}/projects.yaml")
107+
for label in \
108+
"-l purpose=defensive" \
109+
"-l purpose=offensive,purpose=defensive" \
110+
"-l purpose=defensive,purpose=offensive" \
111+
"-l purpose=defensive -l purpose=offensive" \
112+
"-l purpose=offensive -l purpose=defensive"; do
113+
run_sloctl get project "$label"
114+
verify_get_success "$output" "$want"
115+
done
116+
}
117+
118+
@test "projects, labels filtering, AND conditions" {
119+
want=$(read_files "${TEST_INPUTS}/projects.yaml" | yq -r '.[] |= select(.metadata.name == "death-star")')
120+
for label in \
121+
"-l purpose=offensive" \
122+
"-l purpose=defensive,team=vader" \
123+
"-l purpose=offensive,team=vader" \
124+
"-l purpose=offensive,purpose=defensive,team=sidious" \
125+
"-l team=sidious,purpose=offensive,purpose=defensive" \
126+
"-l team=sidious,purpose=defensive,purpose=offensive" \
127+
"-l purpose=offensive -l purpose=defensive,team=sidious" \
128+
"-l purpose=offensive -l team=sidious,purpose=defensive" \
129+
"-l team=sidious -l purpose=offensive -l purpose=defensive" \
130+
"-l purpose=defensive -l purpose=offensive -l team=sidious" \
131+
"-l purpose=offensive -l purpose=defensive -l team=sidious"; do
132+
run_sloctl get project "$label"
133+
verify_get_success "$output" "$want"
134+
done
135+
}
136+
137+
@test "projects, labels filtering with name" {
138+
run_sloctl get project -l purpose=defensive hoth-base
139+
want=$(read_files "${TEST_INPUTS}/projects.yaml" | yq -r '.[] |= select(.metadata.name == "hoth-base")')
140+
verify_get_success "$output" "$want"
141+
142+
run_sloctl get project -l purpose=offensive hoth-base
143+
assert_success
144+
assert_output "No resources found."
145+
}
100146

101147
test_get() {
102148
local \
@@ -145,11 +191,11 @@ test_get() {
145191

146192
for alias in "${aliases[@]}"; do
147193
if [[ "$kind" == "Project" ]] || [[ "$kind" == "UserGroup" ]]; then
148-
run_sloctl get "$alias" "fake-name-123-321"
149-
assert_success
150-
assert_output "No resources found."
194+
run_sloctl get "$alias" "fake-name-123-321"
195+
assert_success
196+
assert_output "No resources found."
151197

152-
continue
198+
continue
153199
fi
154200

155201
run_sloctl get "$alias" "fake-name-123-321"
@@ -170,7 +216,7 @@ verify_get_success() {
170216
# we need to hack our way around it.
171217
refute_output --partial "Available Commands:"
172218
# We can't retrieve the same object we applied so we need to compare the minimum.
173-
filter='[.[] | {"name": .metadata.name, "project": .metadata.project}] | sort_by(.name, .project)'
219+
filter='[.[] | {"name": .metadata.name, "project": .metadata.project, "labels": .metadata.labels}] | sort_by(.name, .project)'
174220
assert_equal \
175221
"$(yq --sort-keys -y -r "$filter" <<<"$have")" \
176222
"$(yq --sort-keys -y -r "$filter" <<<"$want")"

test/inputs/get/projects.yaml

+12
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,24 @@
22
kind: Project
33
metadata:
44
name: death-star
5+
labels:
6+
purpose:
7+
- defensive
8+
- offensive
9+
team:
10+
- vader
11+
- sidious
512
spec:
613
description: Dummy Project for 'sloctl get' e2e tests
714
- apiVersion: n9/v1alpha
815
kind: Project
916
metadata:
1017
displayName: Hoth Base
1118
name: hoth-base
19+
labels:
20+
purpose:
21+
- defensive
22+
team:
23+
- solo
1224
spec:
1325
description: Dummy Project for 'sloctl get' e2e tests

test/test_helper/load.bash

+74-74
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
# The output of sloctl is sanitized, the trailing whitespaces,
1313
# if present, are removed for easier output validation.
1414
run_sloctl() {
15-
run bash -c "set -o pipefail && sloctl $* | sed 's/ *$//'"
15+
run bash -c "set -o pipefail && sloctl $* | sed 's/ *$//'"
1616
}
1717

1818
# read_files
@@ -31,7 +31,7 @@ run_sloctl() {
3131
# documents style.
3232
# yq works with json as it is only a preprocessor for jq.
3333
read_files() {
34-
yq -sY '[ .[] | if type == "array" then .[] else . end]' "$@"
34+
yq -sY '[ .[] | if type == "array" then .[] else . end]' "$@"
3535
}
3636

3737
# assert_applied
@@ -44,7 +44,7 @@ read_files() {
4444
# Options:
4545
# <expected> The expected YAML string.
4646
assert_applied() {
47-
_assert_objects_existence "apply" "$1"
47+
_assert_objects_existence "apply" "$1"
4848
}
4949

5050
# assert_deleted
@@ -57,7 +57,7 @@ assert_applied() {
5757
# Options:
5858
# <expected> The expected YAML string.
5959
assert_deleted() {
60-
_assert_objects_existence "delete" "$1"
60+
_assert_objects_existence "delete" "$1"
6161
}
6262

6363
# _assert_objects_existence
@@ -80,44 +80,44 @@ assert_deleted() {
8080
# - apply: assert that the output contains the expected object.
8181
# - delete: assert that the output contains 'No resources found'.
8282
_assert_objects_existence() {
83-
load_lib "bats-support"
84-
85-
assert [ -n "$2" ]
86-
assert [ "$(yq -r 'type' <<<"$2")" = "array" ]
87-
88-
yq -c .[] <<<"$2" | while read -r object; do
89-
name=$(yq -r .metadata.name <<<"$object")
90-
kind=$(yq -r .kind <<<"$object")
91-
args=("get" "${kind,,}" "$name") # Converts kind to lowercase.
92-
if [[ "$kind" != "Project" ]] && [[ "$kind" != "RoleBinding" ]]; then
93-
project=$(yq -r .metadata.project <<<"$object")
94-
args+=(-p "$project")
95-
fi
96-
97-
case "$1" in
98-
apply)
99-
run_sloctl "${args[*]}"
100-
# shellcheck disable=2154
101-
have=$(yq --sort-keys -y '[.[] | del(.status)]' <<<"$output")
102-
want=$(yq --sort-keys -y '[
83+
load_lib "bats-support"
84+
85+
assert [ -n "$2" ]
86+
assert [ "$(yq -r 'type' <<<"$2")" = "array" ]
87+
88+
yq -c .[] <<<"$2" | while read -r object; do
89+
name=$(yq -r .metadata.name <<<"$object")
90+
kind=$(yq -r .kind <<<"$object")
91+
args=("get" "${kind,,}" "$name") # Converts kind to lowercase.
92+
if [[ "$kind" != "Project" ]] && [[ "$kind" != "RoleBinding" ]]; then
93+
project=$(yq -r .metadata.project <<<"$object")
94+
args+=(-p "$project")
95+
fi
96+
97+
case "$1" in
98+
apply)
99+
run_sloctl "${args[*]}"
100+
# shellcheck disable=2154
101+
have=$(yq --sort-keys -y '[.[] | del(.status)]' <<<"$output")
102+
want=$(yq --sort-keys -y '[
103103
.[] | select(.kind == "'"$kind"'") |
104104
select(.metadata.name == "'"$name"'") |
105105
if .metadata.project then
106106
select(.metadata.project == "'"$project"'")
107107
else
108108
.
109109
end]' <<<"$2")
110-
assert_equal "$have" "$want"
111-
;;
112-
delete)
113-
run_sloctl "${args[*]}"
114-
assert_output --partial "No resources found"
115-
;;
116-
*)
117-
fail "Unknown verb '$1'"
118-
;;
119-
esac
120-
done
110+
assert_equal "$have" "$want"
111+
;;
112+
delete)
113+
run_sloctl "${args[*]}"
114+
assert_output --partial "No resources found"
115+
;;
116+
*)
117+
fail "Unknown verb '$1'"
118+
;;
119+
esac
120+
done
121121
}
122122

123123
# generate_inputs
@@ -137,44 +137,44 @@ _assert_objects_existence() {
137137
# them in parallel or a cleanup after the test fails for whatever reason.
138138
# It works for both YAML and JSON files.
139139
generate_inputs() {
140-
load_lib "bats-support"
140+
load_lib "bats-support"
141141

142-
directory="$1"
143-
test_filename=$(basename "$BATS_TEST_FILENAME" .bats)
144-
TEST_INPUTS="$directory/$test_filename"
145-
mkdir "$TEST_INPUTS"
142+
directory="$1"
143+
test_filename=$(basename "$BATS_TEST_FILENAME" .bats)
144+
TEST_INPUTS="$directory/$test_filename"
145+
mkdir "$TEST_INPUTS"
146146

147-
test_hash="${BATS_TEST_NUMBER}-$(date +%s)-$SLOCTL_GIT_REVISION"
148-
TEST_PROJECT="e2e-$test_hash"
147+
test_hash="${BATS_TEST_NUMBER}-$(date +%s)-$SLOCTL_GIT_REVISION"
148+
TEST_PROJECT="e2e-$test_hash"
149149

150-
files=$(find "$TEST_SUITE_INPUTS/$test_filename" -type f \( -iname \*.json -o -iname \*.yaml -o -iname \*.yml \))
151-
for file in $files; do
152-
pipeline='
150+
files=$(find "$TEST_SUITE_INPUTS/$test_filename" -type f \( -iname \*.json -o -iname \*.yaml -o -iname \*.yml \))
151+
for file in $files; do
152+
pipeline='
153153
if .kind == "Project" then
154-
.metadata.labels = {"origin": ["sloctl-e2e-tests"]}
154+
.metadata.labels.origin = ["sloctl-e2e-tests"]
155155
else
156156
.
157157
end'
158-
filter='
158+
filter='
159159
if type == "array" then
160160
[.[] | '"$pipeline"' ]
161161
else
162162
'"$pipeline"'
163163
end'
164-
new_file="${file/$TEST_SUITE_INPUTS/$directory}"
165-
mkdir -p "$(dirname "$new_file")"
166-
sed_replace="s/<PROJECT>/$TEST_PROJECT/g"
167-
if [[ $file =~ .*.ya?ml ]]; then
168-
yq -Y "$filter" "$file" | sed "$sed_replace" >"$new_file"
169-
elif [[ $file == *.json ]]; then
170-
jq "$filter" "$file" | sed "$sed_replace" >"$new_file"
171-
else
172-
fail "test input file: ${file} must be either YAML or JSON"
173-
fi
174-
done
175-
176-
export TEST_INPUTS
177-
export TEST_PROJECT
164+
new_file="${file/$TEST_SUITE_INPUTS/$directory}"
165+
mkdir -p "$(dirname "$new_file")"
166+
sed_replace="s/<PROJECT>/$TEST_PROJECT/g"
167+
if [[ $file =~ .*.ya?ml ]]; then
168+
yq -Y "$filter" "$file" | sed "$sed_replace" >"$new_file"
169+
elif [[ $file == *.json ]]; then
170+
jq "$filter" "$file" | sed "$sed_replace" >"$new_file"
171+
else
172+
fail "test input file: ${file} must be either YAML or JSON"
173+
fi
174+
done
175+
176+
export TEST_INPUTS
177+
export TEST_PROJECT
178178
}
179179

180180
# select_object
@@ -192,7 +192,7 @@ generate_inputs() {
192192
# extract an object by its former name a regex match with jq 'test'
193193
# function has to be performed.
194194
select_object() {
195-
yq '[if type == "array" then .[] else . end |
195+
yq '[if type == "array" then .[] else . end |
196196
select(.metadata.name | test("^'"$1"'"))]' "$1" "$2"
197197
}
198198

@@ -208,16 +208,16 @@ select_object() {
208208
#
209209
# If 'yq' is provided as one of the dependencies, ensure it is coming from https://github.com/kislyuk/yq.
210210
ensure_installed() {
211-
load_lib "bats-support"
212-
213-
for dep in "$@"; do
214-
if ! command -v "$dep" >/dev/null 2>&1; then
215-
fail "ERROR: $dep is not installed!"
216-
fi
217-
if [ "$dep" = "yq" ] && [ "$(yq --help | grep "kislyuk/yq")" -eq 1 ]; then
218-
fail "ERROR: yq is not installed from https://github.com/kislyuk/yq!"
219-
fi
220-
done
211+
load_lib "bats-support"
212+
213+
for dep in "$@"; do
214+
if ! command -v "$dep" >/dev/null 2>&1; then
215+
fail "ERROR: $dep is not installed!"
216+
fi
217+
if [ "$dep" = "yq" ] && [ "$(yq --help | grep "kislyuk/yq")" -eq 1 ]; then
218+
fail "ERROR: yq is not installed from https://github.com/kislyuk/yq!"
219+
fi
220+
done
221221
}
222222

223223
# load_lib
@@ -230,6 +230,6 @@ ensure_installed() {
230230
# Options:
231231
# <name> Name of the library to load.
232232
load_lib() {
233-
local name="$1"
234-
load "/usr/lib/bats/${name}/load.bash"
233+
local name="$1"
234+
load "/usr/lib/bats/${name}/load.bash"
235235
}

0 commit comments

Comments
 (0)