remotematcher: Implement RemoteMatcher for CRDA #203
Conversation
arajkumar
requested review from
alecmerdler,
hdonnay,
jzelinskie and
ldelossa
as code owners
arajkumar
force-pushed
the
crda-remotematcher-impl
branch
2 times, most recently
from
efcfa0e
to
6d5ae99
Compare
crda/remotematcher.go
Outdated
| const ( | ||
| // Bounded concurrency limit. | ||
| defaultRequestConcurrency = 10 | ||
| defaultURL = "https://f8a-analytics-preview-2445582058137.production.gw.apicast.io/?user_key=3e42fa66f65124e6b1266a23431e3d08" |
There was a problem hiding this comment.
Is this okay for the default for everyone that'll ever use this? It seems odd to have "preview" in the subdomain and a "user_key" URL parameter.
There was a problem hiding this comment.
@hdonnay I had used preview/staging deployment to avoid messing up with our reports.
"user_key" is not a secrete key, it is a publicity known 3scale gateway key.
There was a problem hiding this comment.
@hdonnay @arajkumar how will this work with airgap usage? did we build this into claircore to not call remote matchers if airgap is enabled?
Other then this question, some review comments to look over.
crda/remotematcher.go
Outdated
| if err != nil { | ||
| log.Error().Err(err).Msg("call to component analyses has failed") | ||
| } | ||
| default: |
There was a problem hiding this comment.
@arajkumar I believe you want to remove the select switch and "default" case all together here. It is no longer necessary, you do not want to have a "default" case, you want to unconditionally block on the errorC until it is either closed or an error is present on the channel.
crda/remotematcher.go
Outdated
| Host: reqUrl.Host, | ||
| } | ||
| // A request shouldn't go beyound 10s. | ||
| tctx, cancel := context.WithTimeout(ctx, 10*time.Second) |
There was a problem hiding this comment.
@arajkumar would it be safe to roll this back to 1 second timeout? do you know how long an average API request to your api will take?
There was a problem hiding this comment.
@ldelossa On avg it should take between 2 - 3 seconds, How about 5 seconds?
There was a problem hiding this comment.
I can make it configurable.
There was a problem hiding this comment.
Lets keep it out of config for now, but 5sec sounds good
There was a problem hiding this comment.
Changed it to 5 sec.
I would prefer to leave this upto the configuration instead of turning it off by default for Airgap use. There is a PR in pipeline which will make remote matcher configurable., I will publish once this PR is merged into mainline. |
arajkumar
force-pushed
the
crda-remotematcher-impl
branch
from
b27e81b
to
0ad2004
Compare
arajkumar
force-pushed
the
crda-remotematcher-impl
branch
from
0ad2004
to
c22a6d0
Compare
crda/remotematcher.go
Outdated
|
|
||
| // QueryRemoteMatcher implements driver.RemoteMatcher. | ||
| func (m *Matcher) QueryRemoteMatcher(ctx context.Context, records []*claircore.IndexRecord) (map[string][]*claircore.Vulnerability, error) { | ||
| log := zerolog.Ctx(ctx).With(). |
There was a problem hiding this comment.
@arajkumar can you rebase and use our new logging api:
https://quay.github.io/claircore/contributor/logging.html
There was a problem hiding this comment.
Fixed it. Hope it looks better now.
arajkumar
force-pushed
the
crda-remotematcher-impl
branch
from
c22a6d0
to
a2af60e
Compare
This change brings industry standard Snyk vulnerability data to Clair platform through RedHat's Code Ready Analytics(CRDA) platform[1]. When this is enabled, the libvuln delegates the package to vulnerability matching to a remote service which is hosted by CRDA platform. [1] https://github.com/fabric8-analytics Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
arajkumar
force-pushed
the
crda-remotematcher-impl
branch
from
a2af60e
to
2871483
Compare
This supplements #202.
Tasks