-
Notifications
You must be signed in to change notification settings - Fork 85
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
Configuration groundwork #182
Conversation
c25e066
to
9f85618
Compare
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'd like to see the alpine and aws config ripped out, as changing the config values will have no effect after a layer was scanned.
I think Updater configs need some more thought, do we want a config that looks like?
UpdaterConfigs:
rhel4:
...
rhel5:
...
rhel6:
...
rhel7:
...
ubuntu-disco:
...
etc...
I'd lean toward using the UpdaterSet names that we began using to configure which Updaters runs
UpdaterConfigs:
ubuntu:
...
rhel:
...
oracle
...
internal/indexer/ecosystem.go
Outdated
} | ||
seen[n] = struct{}{} | ||
if _, ok := s.(RPCScanner); airgap && ok { |
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.
can we add an INFO() log to each scanner name which is excluded due to airgap?
@@ -69,7 +70,7 @@ func (ls *layerScanner) Scan(ctx context.Context, manifest claircore.Digest, lay | |||
|
|||
ls.cc = make(chan struct{}, ccMin) | |||
|
|||
ps, ds, rs, err := indexer.EcosystemsToScanners(ctx, ls.Opts.Ecosystems) | |||
ps, ds, rs, err := indexer.EcosystemsToScanners(ctx, ls.Opts.Ecosystems, ls.Opts.Airgap) |
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.
part of me wants the airgap filtering to be it's own method and not be done in EcosystemsToScanner as it'll read a little more declaratively in the construction of the Indexer. what do you think?
libvuln/init.go
Outdated
@@ -28,6 +29,12 @@ func initUpdaters(ctx context.Context, opts *Opts, db *sqlx.DB, store vulnstore. | |||
eC <- fmt.Errorf("duplicate updater found in UpdaterFactory. all names must be unique: %s", u.Name()) | |||
return | |||
} | |||
if c, ok := u.(driver.Configurable); ok { |
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 this is going to take a little more thinking about. This is going to be an issue where we generate the names dynamically like Oracle (generating the date).
internal/indexer/versionedscanner.go
Outdated
@@ -15,6 +21,23 @@ type VersionedScanner interface { | |||
Kind() string | |||
} | |||
|
|||
// ConfigDeserializer can be thought of as a curried Unmarshal function. |
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.
// ConfigDeserializer can be thought of as a curried Unmarshal function. | |
// ConfigDeserializer can be thought of as an Unmarshal function. |
don't believe curried is the right word here as it's just a closure over another method.
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.
Unmarshal
usually has the signature func([]byte, interface{}) error
, so I was trying to point out it's that same concept with the []byte
closed over from a different scope.
libvuln/driver/updater.go
Outdated
type Fingerprint string | ||
|
||
// ConfigUnmarshaler can be thought of as a curried Unmarshal function, or a |
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.
// ConfigUnmarshaler can be thought of as a curried Unmarshal function, or a | |
// ConfigUnmarshaler can be thought of as an Unmarshal function, or a |
same
libvuln/opts.go
Outdated
@@ -59,6 +60,11 @@ type Opts struct { | |||
// | |||
// This list will me merged with the default matchers. | |||
Matchers []driver.Matcher | |||
// Configs is a map of functions for configuration of Updaters. | |||
Configs map[string]driver.ConfigUnmarshaler |
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.
May as well call this UpdaterConfigs if we ever need to pass Matcher configs down the line.
de2f48a
to
c6c1ba0
Compare
// | ||
// This is structured into a closure so that we can type-erase to the | ||
// common VersionedScanner interface and only write all this logic once. | ||
filter := func(s indexer.VersionedScanner) bool { |
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.
Is there any reason this needs to be done at Scan time and not on construction of the layer scanner?
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 don't think so.
// | ||
// This is structured into a closure so that we can type-erase to the | ||
// common VersionedScanner interface and only write all this logic once. | ||
filter := func(s indexer.VersionedScanner) bool { |
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.
large inline function definitions make it pretty tough to understand the gist of the Scan mechanics.
Can this function and the loops below be created as methods with a declarative name so the mechanics of the semaphore usage and concurrent scanning be the focus?
g, ctx := errgroup.WithContext(ctx) | ||
// Scan is a closure to capture the values in the loops and then call the | ||
// scan method. | ||
scan := func(l *claircore.Layer, s indexer.VersionedScanner) func() error { |
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.
can you avoid this scan variable shadow with ls.scan?
put the sem on the layerScanner struct and make it sharable and then use it in ls.scan
seems a bit silly to have ls.scan and then a scan closure just to capture a sem that can be shared in the ls.scan method anyway.
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.
It's not to capture the semaphore, it's to capture the loop variables.
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.
can you make it so the process of performing a scan is self contained in layerScanner.scan?
its simpler then trying to hop around between the Scan function, the scan variable, the function that is returned by the scan variable, the call to the function that is returned by the scan variable, and the ls.scan function.
c6c1ba0
to
263f8cc
Compare
263f8cc
to
86978e1
Compare
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
86978e1
to
7518c5f
Compare
This exploded a bit into also refactoring the layerscanner, which is the primary place where scanners are actually invoked. Signed-off-by: Hank Donnay <hdonnay@redhat.com>
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
7518c5f
to
79bad1e
Compare
These configuration knobs lay the groundwork for having scanners that are configurable and make RPCs, and for making updaters that are configurable.
I'd be fine with dropping the
alpine
commit out of this stack, it's there as a PoC.