Feature: more robust way of logging errors in sub-checks

[laurentsimon]

Today, we return an inconclusive results (accompanied by an error) if a sub-check catches an error. For example, the Pinned-Dependencies check is currently fairly (too!) large, so a single error makes the entire check return -1.

In #1324 (comment), we implemented a heuristic to avoid running the sub-check, using the filename. I feel this approach is fragile. Dockerfile templates are common, and are not always called template, tpl, etc. See #710 and https://raw.githubusercontent.com/kubernetes/kubernetes/master/build/server-image/Dockerfile.

It'd be useful to have a more robust solution. I'n not sure what a better solution is.
Maybe we should create an Error() function to log errors in sub-checks without affecting the entire check result.

Any other ideas?

[laurentsimon]

@chrismcgehee @azeemsgoogle @naveensrinivasan

[laurentsimon]

relevant for shell parsing #1307

[ristomcgehee]

I do think we should simply log when we can't parse a file instead of failing the entire check, essentially do what #1312 does but for any parsing error. It would be nice if we could examine the files that can't be parsed in case we can improve our code to parse better, but inevitably we're going to encounter files that are formatted incorrectly. Maybe we can keep a list of files that we know are not parseable, and just ignore them.

In regards to skipping template Dockerfiles, I think using the filename is a decent start. The file https://raw.githubusercontent.com/kubernetes/kubernetes/master/build/server-image/Dockerfile is not, strictly speaking, a template; it is a fully valid Dockerfile, just with build arguments. To handle a file like this would should enhance are logic to look for hashes as you suggest in #710 (comment), but we shouldn't skip it because it's a template.

[laurentsimon]

I do think we should simply log when we can't parse a file instead of failing the entire check, essentially do what #1312 does but for any parsing error. It would be nice if we could examine the files that can't be parsed in case we can improve our code to parse better, but inevitably we're going to encounter files that are formatted incorrectly. Maybe we can keep a list of files that we know are not parseable, and just ignore them.

that'd be great, yes!

In regards to skipping template Dockerfiles, I think using the filename is a decent start. The file https://raw.githubusercontent.com/kubernetes/kubernetes/master/build/server-image/Dockerfile is not, strictly speaking, a template; it is a fully valid Dockerfile, just with build arguments. To handle a file like this would should enhance are logic to look for hashes as you suggest in #710 (comment), but we shouldn't skip it because it's a template.

Do you know if developers commit their generated dockerfiles from a template?
Can you list a few template dockerfiles that caused problem for scorecard?

In the next iteration of the check, would is still be useful to try to parse the templates and just log a message of parsing fails (like you suggest above)?

[ristomcgehee]

Do you know if developers commit their generated dockerfiles from a template?

I think typically developers use a template dockerfile to build with different architectures or different distros as part of a pipeline. They might sometimes commit the dockerfiles generated from a template, but probably not usually.

Can you list a few template dockerfiles that caused problem for scorecard?

This is the only one I'm aware of that caused errors for scorecard:
https://github.com/traefik/mesh/blob/957da2d9ca4d4cdd4e00d0bdfbb9f5ae4f2685ff/tmpl.Dockerfile
Here's another that would cause have caused errors if we were scanning them:
https://github.com/caddyserver/caddy-docker/blob/62cc558526168fc627b898a3c548c20ce995a8d2/Dockerfile.tmpl

In the next iteration of the check, would is still be useful to try to parse the templates and just log a message of parsing fails (like you suggest above)?

I think that would be a better approach. It looks like some of the dockerfiles that we're now skipping (this one for example) are able to be parsed just fine.

[laurentsimon]

Do you know if developers commit their generated dockerfiles from a template?

I think typically developers use a template dockerfile to build with different architectures or different distros as part of a pipeline. They might sometimes commit the dockerfiles generated from a template, but probably not usually.

Can you list a few template dockerfiles that caused problem for scorecard?

This is the only one I'm aware of that caused errors for scorecard: https://github.com/traefik/mesh/blob/957da2d9ca4d4cdd4e00d0bdfbb9f5ae4f2685ff/tmpl.Dockerfile Here's another that would cause have caused errors if we were scanning them: https://github.com/caddyserver/caddy-docker/blob/62cc558526168fc627b898a3c548c20ce995a8d2/Dockerfile.tmpl

do you know if the templating engine is part of thee official docker tooling or is it provided by a third-party?

In the next iteration of the check, would is still be useful to try to parse the templates and just log a message of parsing fails (like you suggest above)?

I think that would be a better approach. It looks like some of the dockerfiles that we're now skipping (this one for example) are able to be parsed just fine.

[ristomcgehee]

do you know if the templating engine is part of thee official docker tooling or is it provided by a third-party?

It's third-party tooling.

[laurentsimon]

how about creating an Error() function to logs in addition to Warn() and Info()? It would allow logging the error and skip minor parsing issues without returning an error for the entire check. Wdut?

[ristomcgehee]

Yeah, I think that's a good approach.

[laurentsimon]

Added to discussion for next sync.

[github-actions]

This issue is stale because it has been open for 60 days with no activity.