-
Notifications
You must be signed in to change notification settings - Fork 599
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
Add file catalogers to selection configuration #3505
Conversation
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
d919d15
to
bd11b5b
Compare
I'd like to understand a little more context here. It seems like |
That is correct. I also think it's pretty non-obvious to folks that |
removals := strset.New(selectionRequest.RemoveNamesOrTags...) | ||
missingFileIshTag := !defaultNamesOrTags.Has(filecataloging.FileTag) && !defaultNamesOrTags.Has("all") && !defaultNamesOrTags.Has("default") | ||
if missingFileIshTag && !removals.Has(filecataloging.FileTag) { | ||
log.Warnf("adding '%s' tag to the default cataloger selection, to override add '-%s' to the cataloger selection request", filecataloging.FileTag, filecataloging.FileTag) |
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 seems a bit like it says Warning: doing what Syft always did, but now we feel weird about it.
Does this need to be a log.Warn?
Can we just document somewhere that some amount of file cataloging is on unless removed?
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.
what this is trying to convey is what to do next when there is surprising behavior. The rule of thumb for logging here is to only use warn when we have something to direct the user to do to correct a situation. This feels like the right spot for that since we're taking an action to be backwards compatible but could be surprising to folks that already have the sub-selection operation down.
To your point though, I should also update the readme with this context (but I think the warning is good to have too).
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.
@wagoodman I think the README needs a quick update per your previous comment - or is that something to add into the wiki post merge?
FWIW I agree with keeping the warning here.
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.
Under what invocations do we expect the warning to print? --override-default-catalogers ...
? --select-catalogers java
?
It looks like the warning is printed on overriding the default catalogers now, but not otherwise?
I have sort of a general question: Currently, file cataloger selection is controlled by |
Good point -- though it's similar to having a package cataloger configuration to enable data enrichment then de-selecting the cataloger. I think we should allow the action to occur (override configuration with cataloger selection) but we could log-warn in these kinds of situations. |
Note from live stream to take on board feedback from #3468 also. |
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
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 @willmurphyscode had a comment that you responded to regarding a README update, but this LGTM and is important for me to pull in as a part of the refactor for license content PR so 👍
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* main: Add file catalogers to selection configuration (#3505) chore: replace all shorthand tags of mapstruct -> mapstructure (#3633) chore(deps): update tools to latest versions (#3637) chore(deps): update CPE dictionary index (#3638) feat: syft 3435 - add file components to cyclonedx bom output when file metadata is available (#3539) chore(deps): update tools to latest versions (#3635) chore(deps): bump github/codeql-action from 3.28.7 to 3.28.8 (#3634)
Description
This PR adds file catalogers to the cataloger selection configuration:
$ syft cataloger list Default selections: 1 • 'all' Selection expressions: 0 ┌───────────────────────────┬───────────────────────┐ │ FILE CATALOGER │ TAGS │ ├───────────────────────────┼───────────────────────┤ │ file-content-cataloger │ content, file │ │ file-digest-cataloger │ digest, file │ │ file-executable-cataloger │ binary-metadata, file │ │ file-metadata-cataloger │ file, file-metadata │ └───────────────────────────┴───────────────────────┘ ┌────────────────────────────────────────┬───────────────────────────────────────────────────────────────────────────┐ │ PACKAGE CATALOGER │ TAGS │ ├────────────────────────────────────────┼───────────────────────────────────────────────────────────────────────────┤ (same as today...) └────────────────────────────────────────┴───────────────────────────────────────────────────────────────────────────┘
To ensure configuration is backwards compatible,
file
is added to the list of default tags, no matter if the user attempts to override this:Once the user specifies
-file
the warning does not appear:Expressions in the past were applied to all package cataloger tasks, and there was one group of tasks where selections could be applied. This PR adds the ability to apply the selection request to different groups of tasks, applying only the tags and names which a task in the group responds to. Any tags or names that don't apply to tasks within a group are ignored. Any tags or names that were in the original request but were not used against any group results in an error (stop from cataloging). This effectively splits the original user selection request dynamically amongst the group of tasks.
What does this mean in terms of syntax rules? The syntax for cataloger selection does not change. This means that package catalogers and file catalogers can be operated on independently without risk of affecting the one another.
Take for example:
Today this uses the default tag (
image
) and subselects to package catalogers that have thepython
tag and file cataloging stays intact (as evidence of "file metadata", "file digests" and "executables"):Without operating on groups independently and splitting the request, we'd see the same command result in NOT running any file catalogers:
Instead we want a much more explicit expression to remove file cataloging:
This is made possible by always injecting
file
as one of the default tags used for all cataloger selections.file.metadata.selection
tonone
still results in files in the SBOM #2989Type of change
Checklist: