Scan all images under a namespace for Docker remote registries #4514
Conversation
| } | ||
| case dockerScan.FullCommand(): | ||
| if *dockerScanImages != nil && *dockerScanNamespace != "" { | ||
| return scanMetrics, fmt.Errorf("invalid config: you cannot specify both images and namespace at the same time") |
There was a problem hiding this comment.
Why can't we have both images and namespace at the same time?
There was a problem hiding this comment.
We’ve been following this convention across other sources as well. The --images and --namespace flags represent two distinct workflows:
--images: Used to scan one or more explicitly specified images.--namespace: Used to automatically discover and scan all images under a given namespace (organization or user), similar to how the GitHub source distinguishes between--organd--repo.
Both flags can technically overlap, but they are designed for different use cases. Therefore, in the CLI, we allow the user to choose only one of these options per execution just as we do for GitHub’s organization and repository flags to maintain clarity and prevent conflicting inputs.
There was a problem hiding this comment.
This is super cool! I love the interface over the different repos, and the additions to the README are very nice.
I can go into more detail on Slack when I get back from lunch etc., but the changes I want are pretty much all "help this source survive in Enterprise" stuff:
- metrics
- how long our API calls are taking
- how many API call successes and failures
- how many images/histories/layers we enumerate/actually scan
- all errors are logged
- each function/method gets a trace log at level 5 that we're calling it
- integration tests for at least the
ListImagesimplementations- probably just have to mock these w/
gock; I don't think we have a good enough test repo to really test this new functionality
- probably just have to mock these w/
This is a lot of stuff, but I imagine this source will get a lot of use once it can scan registries, so the more we can get ahead of debugging and troubleshooting the better. Also this is mostly my fault; I have "put together logging/metrics guidelines" on my TODO list and I've not gotten to it yet. We'll probably go through another round or two of refinements too, just fair warning haha. But again this is a really good, solid start--core functionality looks perfect.
pkg/sources/docker/registries.go
Outdated
| func MakeRegistryFromNamespace(namespace string) Registry { | ||
| var registry Registry | ||
| switch { | ||
| case strings.HasPrefix(namespace, "quay.io"): // quay.io/abc123 |
There was a problem hiding this comment.
Let's be a little nervous and add an ending "/" like "quay.io/" to rule out like "quay.ioo" or whatever people will do
pkg/sources/docker/registries.go
Outdated
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, | ||
| fmt.Sprintf("https://hub.docker.com/v2/namespaces/%s/repositories", namespace), http.NoBody) |
There was a problem hiding this comment.
I know it's a little annoying, but let's build a net.URL and use like JoinPath to build the `v2/namespaces/%s/repositories" bit -- same for the other repos too.
pkg/sources/docker/docker.go
Outdated
| if namespace := s.conn.GetNamespace(); namespace != "" { | ||
| registry := MakeRegistryFromNamespace(namespace) | ||
|
|
||
| // attach the registry authentication token, if one is available. | ||
| if registryToken := s.conn.GetRegistryToken(); registryToken != "" { | ||
| registry.WithRegistryToken(registryToken) | ||
| } | ||
|
|
||
| ctx.Logger().Info(fmt.Sprintf("using registry: %s", registry.Name())) | ||
|
|
||
| namespaceImages, err := registry.ListImages(ctx, namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list namespace images: %w", err) | ||
| } | ||
|
|
||
| ctx.Logger().Info(fmt.Sprintf("namespace: %s has %d images", namespace, len(namespaceImages))) | ||
|
|
||
| s.conn.Images = append(s.conn.Images, namespaceImages...) |
There was a problem hiding this comment.
I like keeping it simple here and piggybacking on s.conn.Images, but I think it'll make integration testing a little tricky because we're making network calls in Init here. My first idea for making this easier is to break it out into a helper like getImages that Chunks can call, but there's other options.
kashifkhan0771
force-pushed
the
feature/ins-58
branch
from
92caf1e to
9db552f
Compare
Added metrics to track API call durations.
We already have existing metrics for the number of images scanned.
All errors within Chunk() are now logged.
Done ✅ where we are passing context
I already added test cases for ListImages in registries_test.go, covering all registries using public namespaces. |
kashifkhan0771
force-pushed
the
feature/ins-58
branch
from
8fbab73 to
daebef8
Compare
3 participants
Description:
This PR add support for scanning images under a namespace (Org or user) in remote registries.
Supported Remote Registries:
Checklist:
make test-community)?make lintthis requires golangci-lint)?