goss-org / goss

Quick and Easy server testing/validation
https://goss.rocks
Apache License 2.0
5.55k stars 473 forks source link

render silently drops overlapping directives #743

Closed plannigan closed 11 months ago

plannigan commented 2 years ago

Describe the bug Since the goss file format is a dictionary, overlapping keys do not work. This is described in the manual. However, when using multiple goss files, it is less obvious when this happens. Additionally I have a use case where it is helpful to break checks out into multiple logical categories that need to use the same directives to assert different things.

When render is used to combine files that have these types of overlaps, it does produce a valid file. But it silently drops values that appear first.

How To Reproduce Below is a minimal example that reproduces the issue. Checking environment variable can be worked around using exec and providing distinct names. But splitting multiple checks for a specific file across files can never work.

$ cat a.yaml 
command:
  env:
    exit-status: 0
    stdout:
      - HOME=
file:
  /etc/passwd:
    exists: true
    owner: root

$ cat b.yaml 
command:
  env:
    exit-status: 0
    stdout:
      - PWD=
file:
  /etc/passwd:
    exists: true
    group: root

$ cat combined.yaml 
gossfile:
  a.yaml: {}
  b.yaml: {}

$ ./goss-linux-amd64 -g combined.yaml render
file:
  /etc/passwd:
    exists: true
    group: root
    contains: []
command:
  env:
    exit-status: 0
    stdout:
    - PWD=
    stderr: []
    timeout: 0

$ echo $?
0

Expected Behavior In an idea world, I'd like the checks to be merged if they do not conflict (which is true in the example & my actual use case). However, I understand that is much more complicated to implement and might be surprising behavior to others.

Being more realistic, I'd expect render to exit with an error because it can't produce a valid goss file that accurately represents all of the files being combined.

Actual Behavior render produces a valid goss file that does not include all of the checks from the combined files with no indication that this happened.

Environment:

ekelali commented 1 year ago

I don't think this is a bug, but rather intentional behavior to allow for overrides. See:

https://github.com/aelsabbahy/goss/issues/150

Perhaps this can be an optional flag for render or something (perhaps --strict?) that's printed out when CLI verbosity is increased?

GhuulCZ commented 12 months ago

If this is not a bug then it is major flaw. If you are using goss for validation then the purpose of validation tool is to report what was checked with what result but also what was not checked. If you get inconsistent results because on one machine the dictionary is assembled differently then on another and goss does not complain about it (this should be at least INFO, WARN would be better) then you have a problem. If there is a need for "overrides" then I would vote for having the ability to specify it through optional parameter.

aelsabbahy commented 12 months ago

Hello @GhuulCZ , thank you for the feedback, I'd like to understand the issue further to see if a solution can be implemented that works for all.

Possible bug:

If you get inconsistent results because on one machine the dictionary is assembled differently then on another

This was fixed long ago with: https://github.com/goss-org/goss/pull/155 are you seeing non-deterministic behavior with the same input? If so, that is definitely a bug.

Feature enhancement:

If the input is different, I would like to brainstorm a bit on a fix that would both allow overrides and remove the footgun:

  1. There's a few locations where conflicting names can happen, currently goss allows for both: a. within the same file b. across files
  2. Adding logging/warn messages
  3. adding a flag (--strict, --no-clobber, --no-overrides, etc.) that prevents keys from being overwritten for both render and validate commands.

I'm wondering if adding 3 would solve both 1a and 1b and make 2 unnecessary?

GhuulCZ commented 11 months ago

Hi @aelsabbahy , sorry for late reply.

If you get inconsistent results because on one machine the dictionary is assembled differently then on another

We did fixed the inconsistency by specifically set order for each yaml which is included, as usual there were previously wildcard chars used which is not a good practice anyway I belive

2. Adding logging/warn messages

For me it is clear now how to avoid such overlapping issue and we did rewrite all the affected tests however I would say that some info/warn message would be still useful as "missing" test can be easily overlooked especially when you are working with large set of controls with multiple people involved where it is not uncommon that each person is validating only its own part. Anyway lesson learned and we have incorporated validation script that checks all GOSS results and reports those which are missing (validator for validator ;-))