Command optional exec style

[berney]

Fixes #870

Checklist

• make test-linux-all (UNIX) passes. CI will also test this
• Windows platform
• unit and/or integration tests are included (if applicable)
• documentation is changed or added (if applicable)
• Update goss yaml schema
• Update goss JSON schema

Description of change

This change allows specifying a command to be executed exec style without a shell.
This change is backwards compatible, and existing goss configuration files should continue to work as expected.
This change is needed for operating in environments where a shell doesn't exist, such as testing a distroless docker image.
The syntax is inspired by the Dockerfile RUN, ENTRYPOINT, CMD instructions that can take either a string (shell style), or a list (exec style). This allows specifying arguments that contain spaces, etc, as the argument list is explicit.

Shell syntax (unchanged from before):

command:
  figlet:
    exit-status: 0
    exec: figlet test

This would be wrapped and executed as sh -c "figlet test"

Exec syntax (this change introduces):

command:
  figlet:
    exit-status: 0
    exec:
      - figlet
      - test

This would be executed directly, without a shell, as figlet test, where figlet is arg 0 and test is arg 1.

The above could use alternative yaml syntax:

command:
  figlet:
    exit-status: 0
    exec: [figlet, test]

[berney]

@aelsabbahy I've got this working on Linux for shell and exec style commands. I've made it draft as I'm unsure of a few things. If you have the time I'd appreciate some feedback.

Once I have this change in acceptable form, I want to add:

1. Ability to specify more os/exec attributes like environment variables, re Allow setting environment variables for command tests #602
2. Setting current working directory

This would expose more of https://pkg.go.dev/os/exec#Cmd to goss Command resource.
I could open separate PRs or bundle the changes in the one. Do you have a preference?

So far, I've just worked on posix platform.
I will work on Windows when I'm on the right track.

The existing code in system/system.go, and can only pass a single string from resource to system (besides context.Context, *System, and util2.Config).
In the change I made, I have it pass a struct util2.ExecCommand, which should either have a string or a slice.
I added code to unmarshal this struct from json/yaml.

I couldn't work out how to properly using genny, so I have manually updated resource/resource_list.go.
I know this is wrong way to do it.
Any tips on proper way to get this to work?

I feel there are a lot of similar types in the files:

• resource/command.go: type Command struct
• util/command.go: type Command struct
• system/command.go: type DefCommand struct
• util/command.go: type ExecCommand struct (new, that I've added)
• and there's pkg os/exec Cmd struct as well.

and there is code between resource, system, and util, to covert between the similar types.
I'm not sure if this can be refactored into better code - I'm a beginner at Go programming.

Later when I expand on this work, I'll want to be passing more data, env vars and working directory.

Also, in system/system.go it imports util as util2, I don't understand why it needs to alias it, why can't it default to util?

[berney]

I'm looking into the ci/build.sh and test failures with the unmarshal errors.

[berney]

Fixed, so passes tests in CI now.

For me locally it fails:

INFO: Starting build wheezy
cd integration-tests/ && ./test.sh wheezy amd64
+ os=wheezy
+ arch=amd64
+ vars_inline='{inline: bar, overwrite: bar}'
+ cd integration-tests
+ cp ../release/goss-linux-amd64 goss/wheezy/
+ md5sum -c Dockerfile_wheezy.md5
Dockerfile_wheezy: OK
+ docker images
+ grep aelsabbahy/goss_wheezy
aelsabbahy/goss_wheezy                                                 latest            c27c7ff420e2   16 minutes ago   277MB
+ container_name=goss_int_test_wheezy_amd64
+ docker ps -a
+ grep goss_int_test_wheezy_amd64
+ network=goss-test
+ docker network create --driver bridge --subnet 172.19.0.0/16 goss-test
cdf15369ed77f4c6ecdc9e726830abd51779572ec3474ae1f7baf0099d6057eb
+ docker run -d --name httpbin --network goss-test kennethreitz/httpbin
2e877272d56d25a98e4525d4c424b15e054387867ab3071360d49882bcc98805
+ opts=(--env OS=$os --cap-add SYS_ADMIN -v "$PWD/goss:/goss" -d --name "$container_name" --security-opt seccomp:unconfined --security-opt label:disable)
++ docker run --env OS=wheezy --cap-add SYS_ADMIN -v /home/berne/co/git/goss/integration-tests/goss:/goss -d --name goss_int_test_wheezy_amd64 --security-opt seccomp:unconfined --security-opt label:disable --network goss-test aelsabbahy/goss_wheezy /sbin/init
+ id=19a6f40c0eefdc6885a9309293fee69b022a45a48cc9a53c8a3517ad085aee6a
++ docker inspect --format '{{ .NetworkSettings.IPAddress }}' 19a6f40c0eefdc6885a9309293fee69b022a45a48cc9a53c8a3517ad085aee6a
+ ip=
+ trap 'rv=$?; docker rm -vf 19a6f40c0eefdc6885a9309293fee69b022a45a48cc9a53c8a3517ad085aee6a;docker rm -vf httpbin;docker network rm goss-test; exit $rv' INT TERM EXIT
+ [[ wheezy != \a\r\c\h ]]
+ docker_exec /goss/wheezy/goss-linux-amd64 -g /goss/goss-wait.yaml validate -r 10s -s 100ms
+ docker exec goss_int_test_wheezy_amd64 /goss/wheezy/goss-linux-amd64 -g /goss/goss-wait.yaml validate -r 10s -s 100ms
FF

Failures/Skipped:

Addr: tcp://localhost:80: reachable:
Expected
    false
to equal
    true

Addr: tcp://localhost:8888: reachable:
Expected
    false
to equal
    true

Total Duration: 0.002s
Count: 2, Failed: 2, Skipped: 0
Retrying in 100ms (elapsed/timeout time: 0.003s/10s)

Attempt #48:
FF

Failures/Skipped:

Addr: tcp://localhost:80: reachable:
Expected
    false
to equal
    true

Addr: tcp://localhost:8888: reachable:
Expected
    false
to equal
    true

Total Duration: 0.001s
Count: 2, Failed: 2, Skipped: 0
Retrying in 100ms (elapsed/timeout time: 4.832s/10s)


++ docker_exec /goss/wheezy/goss-linux-amd64 --vars /goss/vars.yaml --vars-inline '{inline: bar, overwrite: bar}' -g /goss/wheezy/goss.yaml validate
++ docker exec goss_int_test_wheezy_amd64 /goss/wheezy/goss-linux-amd64 --vars /goss/vars.yaml --vars-inline '{inline: bar, overwrite: bar}' -g /goss/wheezy/goss.yaml validate
Error response from daemon: container 19a6f40c0eefdc6885a9309293fee69b022a45a48cc9a53c8a3517ad085aee6a is not running
+++ _err_trap
+++ local err=1
+++ local 'cmd=docker exec "$container_name" "$@"'
+++ set +x
panic: uncaught error
Traceback (most recent call first):
  at ./test.sh:26 in docker_exec()
  at ./test.sh:47 in main()
docker exec "$container_name" "$@" exited 1
panic: uncaught error
Traceback (most recent call first):
  at ./test.sh:47 in main()
docker exec "$container_name" "$@" exited 1
+ out=
++ _err_trap
++ local err=1
++ local 'cmd=out=$(docker_exec "/goss/$os/goss-linux-$arch" --vars "/goss/vars.yaml" --vars-inline "$vars_inline" -g "/goss/$os/goss.yaml" validate)'
++ set +x
panic: uncaught error
Traceback (most recent call first):
  at ./test.sh:47 in main()
out=$(docker_exec "/goss/$os/goss-linux-$arch" --vars "/goss/vars.yaml" --vars-inline "$vars_inline" -g "/goss/$os/goss.yaml" validate) exited 1
19a6f40c0eefdc6885a9309293fee69b022a45a48cc9a53c8a3517ad085aee6a
httpbin
goss-test
make: *** [Makefile:120: wheezy] Error 1

But the fix in code looked correct to me, and in CI it passes. So I think it is something to do with my local setup.

[aelsabbahy]

I'm a bit mixed on this UX. I understand it, it makes sense, is very elegant, which makes me love it. However, it does mean that command is a string or an array of strings.. which no other resource does, and it can't be used with goss add .., not sure what other implications there are if any.

The resource/resource_list.go part does need to be figured out since that contract is the same across all resources, with all of them taking a string.

I don't have an easy fix for you to be honest. Is a solution possible if NewCommand took an interface and then type asserted it to a string or []string? I'm on my phone, so didn't dig deep into the code to see if it's viable or not.

Assuming we can get the codebase to corporate without any odd foot guns, this does seem to be an extremely elegant solution.

[berney]

However, it does mean that command is a string or an array of strings.. which no other resource does, and it can't be used with goss add .., not sure what other implications there are if any.

goss add command ... still works in that it has the same old behaviour, it will be the shell style.
At the moment the, only way to get exec style is to manually edit the yaml.
Do you think this is acceptable? Or do you think there should be a way to add exec style commands with goss add command as well?

Is a solution possible if NewCommand took an interface and then type asserted it to a string or []string?

At the moment I made it take a struct instead of a string:

func NewDefCommand(ctx context.Context, command util.ExecCommand, system *System, config util.Config) Command {

and the code looks at the attributes of the struct.
I then did a dirty temporary workaround for the autogenerated code.

Perhaps an interface and type assertions like you suggest would work too, to make all the code use the same function prototype.

Thanks for the feedback and ideas.

[aelsabbahy]

Or do you think there should be a way to add exec style commands with goss add command as well?

Reflecting on this.. I don't think it's necessary. It's fine if adding by cli only uses shell.. and only way to handle exec is by editing the YAML.

Let's try the interface change or any approach that won't require manual edits to resource_list.go as all the code in there is generated and similar across all resource types.

[berney]

@aelsabbahy I've implemented the interface{} idea. This fixes the problem I had with genny and the resource/resource_list.go.

The interface{} means that any type can be passed, such as string, or a string slice []string, or any other type (which would be unexpected). At run time I check the types. So, I changed the function prototype to return an error. But in most cases I don't actually check the error (I'll come back to this).

All goss checks uses only strings, except command which can be either a string or a string slice (for the exec style, the point of this PR). In the resource/command.go I do check if the sys.NewCommand had an error. I wasn't sure of best way to handle the error, I create a TestResult with Result: FAIL. I'm not sure if there's a better or more goss idiomatic way to do this.

I can update the rest of the resources to check if there was an error, rather than ignore it.

I think the genny code could be replaced with native Go generics introduced in Go v1.18, and possibly other parts of the code could use generics too. Thus far I think this is the only bit of code using them:

goss/goss_config.go

Line 114 in e73553f

func mergeType[V any](m map[string]V, t, k string, v V) {

I think generics would be a better solution because IIUC the type checking would happen at build time, based on type constraints, rather than at runtime. Because the type checking would happen at build time, runtime checks shouldn't be needed, and so checking for returned errors wouldn't be needed either.

With the interface{} and runtime checking, programming mistakes could result in successful builds, but bugs at runtime, and these might not be covered by automated testing. For example, I had a bug at first where I returned the wrong type in the c.id case, but luckily the tests covered it: https://app.travis-ci.com/github/goss-org/goss/jobs/617952958. I fixed this in c129fd4.

I tested locally that the exec style commands are working. I will be adding unit tests in this PR.

1. What do you think about rewriting resource_list.go to use Go generics?
2. Should I do that in a separate PR or in this one?
3. Do it first or get this merged and then do the Go generics refactor? (I'm thinking generics first).

After the implementation is solidified, I'll work on the remaining tasks (Error handling for all resource types (if needed), Unit/Integration Tests, Docs, json and yaml schemas).

[aelsabbahy]

Looked over the code changes. Some questions:

1. Why not just have the NewCommand take the interface{} and leave everything else with string argument. This would reduce the runtime risk to only the one check that has a dynamic type perhapse this can be eliminated later, with generics. Curious on your opinion on this prior to more changes.

2. I'm 100% for a generics re-write. A few minor things to consider: Goss predated generics, Genny is more flexible then generics.. that's not to say I want to keep Genny (I don't), but that the translation won't be a simple 1:1. My vote is for a follow up PR for genetics

3. Minor nitpick interface{} -> any

[aelsabbahy]

Looping back on this, any update?