Open berney opened 5 months ago
@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:
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:
type Command struct
type Command struct
type DefCommand struct
type ExecCommand struct
(new, that I've added)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
?
I'm looking into the ci/build.sh and test failures with the unmarshal errors.
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.
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.
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:
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.
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.
@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: https://github.com/goss-org/goss/blob/e73553f9c3065ac297499dafb4f8abef6acb24ad/goss_config.go#L114
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 c129fd41c0f759e8e4a557c878c623ceaabf4195.
I tested locally that the exec style commands are working. I will be adding unit tests in this PR.
resource_list.go
to use Go generics?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).
Looked over the code changes. Some questions:
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.
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
Minor nitpick interface{}
-> any
Looping back on this, any update?
Fixes #870
Checklist
make test-linux-all
(UNIX) passes. CI will also test thisDescription 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):
This would be wrapped and executed as
sh -c "figlet test"
Exec syntax (this change introduces):
This would be executed directly, without a shell, as
figlet test
, wherefiglet
is arg 0 andtest
is arg 1.The above could use alternative yaml syntax: