-
Notifications
You must be signed in to change notification settings - Fork 711
add filter option to list command #4187
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: master
Are you sure you want to change the base?
add filter option to list command #4187
Conversation
4601c97
to
c7631d4
Compare
cmd/limactl/list.go
Outdated
} | ||
if len(filter) != 0 { | ||
for _, f := range filter { | ||
yq = append(yq, "select("+f+")") |
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.
Needs escaping
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.
I tested numerous escaping approaches as asked, but it breaks the yq expressions. What string escape approach would you recommend? Thanks @AkihiroSuda
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.
@AkihiroSuda I don't see why this needs escaping. The filter expression should be inserted literally inside the select()
.
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.
Without escaping the code looks vulnerable to "Bobby Tables".
Unlikely to result in breaking data in our case though
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.
How is this different from any code you specify via --yq
? The only difference is that the --filter
argument is wrapped inside a select()
call.
The user can literally send the same command via --yq "$(printf "select(%s)" "$FILTER")"
.
Any escaping would simply break the filter expression.
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.
Side note: the --filter
command can only expose the environment variables when used with JSON or YAML output.
For table or Go templates the filter expression is just evaluated to check if the instance is to be included or not, but the command will print the original instance data, not the one potentially modified by the --filter
expression. So it can only extract 1 bit of information per call.
But calling os.Clearenv()
makes all of this irrelevant anyways.
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.
Second side note: I also didn't see any functionality in yq
that would allow you to get a list of all environment variables. You have to know the variable name to query the value. Please let me know if I overlooked anything!
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.
Clearing the env may cause an unexpected effect to the entire process, including the Go runtime.
Can't we just add a new option in the YQ library to prohibit accessing envs?
At least https://pkg.go.dev/github.com/mikefarah/yq/[email protected]/pkg/yqlib#ExpressionNode could be verified that there is no reference to env vars.
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.
Clearing the env may cause an unexpected effect to the entire process, including the Go runtime.
All we do after clearing the environment is calling yqlib
to evaluate the expression, printing the result with fmt.Fprint
, and exiting the process. I can't see how anything would be affected in an unexpected way.
The fact that os.Getenv
will always return the empty string inside yqlib
is not unexpected, but desired.
I would be opposed to modifying the process environment for every yqlib
call, but in limactl list
and limactl info
it would be acceptable when all that is left to do is evaluating an expression and printing the result.
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.
Can't we just add a new option in the YQ library to prohibit accessing envs?
I don't know; there doesn't seem to be an easy way to pass settings to the env
operator etc.
I agree that this would be a good enhancement (so we could make it apply to all our yqlib
calls), but for limactl list
it seems like overkill because I don't see an easy way to implement it.
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.
Needs changes (allow --filter
with --yq
) and simplifications.
cmd/limactl/list.go
Outdated
if len(yq) != 0 { | ||
return errors.New("option --filter conflicts with option --yq") | ||
} |
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.
I think you should be able to combine --filter
and --yq
as long as the output format is json
or yaml
.
So you could just drop the check here because yq
is already incompatible with the other output formats.
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.
This isn't too clear to me but is there anything wrong with the way it currently is?
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.
Yes:
❯ l ls -l '.status == "Running"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
❯ l ls -l '.status == "Running"' --yq .name
FATA[0000] option --filter conflicts with option --yq
❯ l ls --yq 'select(.status == "Running")' --yq .name
alpine-iso
I expect the last 2 commands to work exactly the same. There is no reason for the len(yq)
check to exist.
cmd/limactl/list.go
Outdated
} | ||
if len(filter) != 0 { | ||
for _, f := range filter { | ||
yq = append(yq, "select("+f+")") |
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.
@AkihiroSuda I don't see why this needs escaping. The filter expression should be inserted literally inside the select()
.
404b08c
to
a5ba9ad
Compare
cmd/limactl/list.go
Outdated
isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}") | ||
if len(filter) != 0 && (format == "table" || isGoTemplate) { |
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.
isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}") | |
if len(filter) != 0 && (format == "table" || isGoTemplate) { | |
if len(filter) != 0 && format != "json" && format != "yaml") { |
Despite what the --help
output says, every string except table
, json
, and yaml
is treated as a Go template, so the check for the double braces prefix/suffix is not correct:
❯ l ls -f 'Name is: {{.Name}}'
Name is: alpine-iso
Name is: alpine-lima-3.22.2
Name is: foo
cmd/limactl/list.go
Outdated
for _, instance := range instances { | ||
jsonBytes, err := json.Marshal(instance) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err) |
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.
return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err) | |
return nil, fmt.Errorf("failed to marshal instance %q: %w", instance.Name, err) |
Always use %q
when printing use input.
Also applies to the next error message.
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.
224e6e4
to
b3f18db
Compare
Signed-off-by: olalekan odukoya <[email protected]>
b3f18db
to
bdef056
Compare
Examples: | ||
--filter '.status == "Running"' | ||
--filter '.vmType == "vz"' | ||
--filter '.status == "Running"' --filter '.vmType == "vz"' |
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.
Is this AND-matching or OR-matching?
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.
I assume you want the description updated to clarify.
It is AND so you can do this:
❯ cat ~/bin/limactl-ps
#!/bin/sh
limactl ls --filter 'select(.status == "Running")' "$@"
❯ l ls
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
alpine-lima-3.22.2 Stopped 127.0.0.1:0 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-lima-3.22.2
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
foo Stopped 127.0.0.1:0 vz aarch64 4 1.177MiB 100GiB ~/.lima/foo
❯ l ps
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
❯ l ps -l '.vmType == "vz"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
if !valid.MatchString(f) { | ||
return fmt.Errorf("unsafe characters in filter expression: %q", f) | ||
} |
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.
I don't think this is useful. The whole point of --filter
and --yq
is to provide YQ expressions that are going to be evaluated. There is no vulnerability; the expression is provided by the user themselves.
And we already get an error when the YQ expression is invalid, e.g.
❯ l ls -l '.vmType =='
FATA[0000] failed to apply filter select(.vmType ==): '==' expects 2 args but there is 1
So I don't understand the purpose of this "validation", especially since it might make legitimate use cases invalid (e.g. you are restricting which characters are allowed inside literal strings).
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.
The combination of --filter
and --quiet
is not working (the filter is ignored):
❯ l ls -l '.vmType == "qemu"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
❯ l ls -l '.vmType == "qemu"' -q
alpine-iso
alpine-lima-3.22.2
default
foo
Fixes