feat: [CN-296] Support Container Base Image Recommendations #991
Conversation
|
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
| agg.referenceScanStates[folderProductKey{Product: product.ProductOpenSource, FolderPath: folderPath}] = &scanState{Status: NotStarted} | ||
| agg.referenceScanStates[folderProductKey{Product: product.ProductCode, FolderPath: folderPath}] = &scanState{Status: NotStarted} | ||
| agg.referenceScanStates[folderProductKey{Product: product.ProductInfrastructureAsCode, FolderPath: folderPath}] = &scanState{Status: NotStarted} | ||
| agg.referenceScanStates[folderProductKey{Product: product.ProductContainer, FolderPath: folderPath}] = &scanState{Status: NotStarted} |
There was a problem hiding this comment.
Does your implementation support delta scanning? If not, this probably needs a revision.
There was a problem hiding this comment.
The whole API client functionality should be a workflow in https://github.com/snyk/go-application-framework and used / imported here. That would enable re-use in the CLI and migration away from the legacy CLI functionality through using a go-workflow.
|
|
||
| // Add headers | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("x-snyk-ide", "snyk-ls-"+config.Version) |
There was a problem hiding this comment.
This may already be done automatically during LS initialization, I assume this does not need to be done manually here.
| apiURL := fmt.Sprintf("%s/docker-deps/recommended-base-image", d.config.SnykApi()) | ||
|
|
||
| // Create the request | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) |
There was a problem hiding this comment.
As container scanning is a long running process (I think), is there a way to report progress e.g. polling progress from the API?
| // Add authentication header | ||
| if d.config.NonEmptyToken() { | ||
| if d.config.AuthenticationMethod() == types.OAuthAuthentication { | ||
| oAuthToken, tokenErr := d.config.TokenAsOAuthToken() | ||
| if tokenErr == nil { | ||
| req.Header.Set("Authorization", "Bearer "+oAuthToken.AccessToken) | ||
| } | ||
| } else { | ||
| req.Header.Set("Authorization", "token "+d.config.Token()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is done automatically, if you use an authenticated client. No need to add the header here.
| func (sc *Scanner) Scan(ctx context.Context, path types.FilePath, folderPath types.FilePath, folderConfig *types.FolderConfig) (issues []types.Issue, err error) { | ||
| fileInfo, err := os.Stat(string(path)) | ||
| if err != nil { | ||
| sc.errorReporter.CaptureError(err) |
There was a problem hiding this comment.
Better use CaptureErrorAsIssue
| issues := make([]types.Issue, 0) | ||
|
|
||
| // Regex to match FROM instructions in Dockerfile | ||
| fromRegex := regexp.MustCompile(`(?i)^\s*FROM\s+([^\s]+)`) |
There was a problem hiding this comment.
premature optimization from my side: this should be a global variable/constant
| lowerFileName == "docker-compose.yaml" | ||
| } | ||
|
|
||
| func (sc *Scanner) scanDockerFile(ctx context.Context, filePath string, content string) ([]types.Issue, error) { |
There was a problem hiding this comment.
This is missing context, requestID & progress handling. Have a look at the other scanners for that :)
| for lineNum, line := range lines { | ||
| matches := fromRegex.FindStringSubmatch(line) | ||
| if matches == nil { | ||
| continue | ||
| } | ||
|
|
||
| baseImage := matches[1] | ||
| // Skip scratch and multi-stage build references | ||
| if baseImage == "scratch" || strings.Contains(baseImage, " as ") { | ||
| continue | ||
| } | ||
|
|
||
| // Get recommendation from API | ||
| recommendation, err := sc.apiClient.GetBaseImageRecommendation(ctx, baseImage) | ||
| if err != nil { | ||
| sc.errorReporter.CaptureError(err) | ||
| log.Err(err).Str("method", "container.scanDockerFile").Msg("Error getting base image recommendation") | ||
| continue | ||
| } | ||
|
|
||
| // Create issue with code action to replace base image | ||
| issue := sc.createBaseImageIssue(filePath, lineNum, line, baseImage, recommendation) | ||
| issues = append(issues, issue) | ||
| } |
There was a problem hiding this comment.
I'd restructure the logic into: input (=image extraction), processing (api call), output (conversion to issues)
There was a problem hiding this comment.
Also, I'd parallelize the API calls with goroutines.
|
|
||
| func (sc *Scanner) createBaseImageIssue(filePath string, lineNum int, line string, baseImage string, recommendation *BaseImageRecommendation) types.Issue { | ||
| // Find the position of the base image in the line | ||
| fromIndex := strings.Index(strings.ToLower(line), "from") |
There was a problem hiding this comment.
There's an AST package, you could use to create an AST out of the dockerfile and use that afterwards. We already do that for maven. You would add a dockerfile parser and a Parser interface that is implemented by both Maven parser and Dockerfile parser with Parse(content string, path types.FilePath) *ast.Tree as function. WDYT?
| } | ||
|
|
||
| codeAction, err := snyk.NewCodeAction( | ||
| fmt.Sprintf("Upgrade to %s", recommendation.RecommendedImage), |
There was a problem hiding this comment.
We may want to add (Snyk) at the end of the action title.
There was a problem hiding this comment.
Also, in Open Source, we add the number of issues that would be fixed, and the number of issues that would remain in the text. Having that for container, would be great!
| // Create issue | ||
| issue := &snyk.Issue{ | ||
| ID: fmt.Sprintf("SNYK-CONTAINER-BASE-IMAGE-%s", baseImage), | ||
| Severity: types.Medium, |
There was a problem hiding this comment.
I'd suggest the highest severity of an issue found within the baseImage
|
|
||
| // Create issue | ||
| issue := &snyk.Issue{ | ||
| ID: fmt.Sprintf("SNYK-CONTAINER-BASE-IMAGE-%s", baseImage), |
There was a problem hiding this comment.
the ID should be unique - why only one issue per baseImage?
| Message: fmt.Sprintf("Base image '%s' may have security vulnerabilities", baseImage), | ||
| FormattedMessage: sc.getFormattedMessage(baseImage, recommendation), | ||
| AffectedFilePath: types.FilePath(filePath), | ||
| ContentRoot: types.FilePath(filePath), |
There was a problem hiding this comment.
ContentRoot must be the folder
| } | ||
|
|
||
| func TestScanner_Product(t *testing.T) { | ||
| c := config.New() |
There was a problem hiding this comment.
we have test helpers that help with setting up a new config, e.g. c := testutil.UnitTest(t) for a unit test or c:= testutil.SmokeTest(t) for a smoke test that tests E2E.
dan-arpino
force-pushed
the
feat/CN-296-container-base-image-rec
branch
from
380b07d to
701895d
Compare
dan-arpino
force-pushed
the
feat/CN-296-container-base-image-rec
branch
from
701895d to
38fe982
Compare
Adding a new Container LS to support the upgrading the container base images in a docker file.
dan-arpino
force-pushed
the
feat/CN-296-container-base-image-rec
branch
from
38fe982 to
9052808
Compare
Adding a new Container LS to support the upgrading the container base images in a docker file.
Description
Adding base image recommendations to the IDE. This will scan docker files for base images and check if we have any base image upgrade recommendations CN-296
Checklist
make generate)make lint-fix)