cmd/frontend, internal/vuln: add vulndb v1 experiment
Add an experiment, "vulndb-v1", which if active causes pkgsite to
read from the v1 vulnerability database instead of the legacy database.
The experiment is not yet enabled anywhere.
For golang/go#58928
Change-Id: I66d6a90fc2eb841ed674169c09ea36c957551f1b
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/476556
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Run-TryBot: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
diff --git a/internal/experiment.go b/internal/experiment.go
index e2681b7..5c2e1af 100644
--- a/internal/experiment.go
+++ b/internal/experiment.go
@@ -8,6 +8,7 @@
const (
ExperimentEnableStdFrontendFetch = "enable-std-frontend-fetch"
ExperimentStyleGuide = "styleguide"
+ ExperimentVulndbV1 = "vulndb-v1"
)
// Experiments represents all of the active experiments in the codebase and
@@ -15,6 +16,7 @@
var Experiments = map[string]string{
ExperimentEnableStdFrontendFetch: "Enable frontend fetching for module std.",
ExperimentStyleGuide: "Enable the styleguide.",
+ ExperimentVulndbV1: "Use the v1 vulnerability database instead of the legacy database.",
}
// Experiment holds data associated with an experimental feature for frontend
diff --git a/internal/vuln/client.go b/internal/vuln/client.go
index 58373cb..b512459 100644
--- a/internal/vuln/client.go
+++ b/internal/vuln/client.go
@@ -8,21 +8,40 @@
"context"
"fmt"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/derrors"
+ "golang.org/x/pkgsite/internal/experiment"
vulnc "golang.org/x/vuln/client"
"golang.org/x/vuln/osv"
)
-// Client reads Go vulnerability databases.
+// Client reads Go vulnerability databases from both the legacy and v1
+// schemas.
+//
+// If the v1 experiment is active, the client will read from the v1
+// database, and will otherwise read from the legacy database.
type Client struct {
legacy *legacyClient
- // v1 client, currently for testing only.
- // Always nil if created via NewClient.
- v1 *client
+ v1 *client
}
// NewClient returns a client that can read from the vulnerability
// database in src (a URL representing either a http or file source).
func NewClient(src string) (*Client, error) {
+ // Create the v1 client.
+ var v1 *client
+ s, err := NewSource(src)
+ if err != nil {
+ // While the v1 client is in experimental mode, ignore the error
+ // and always fall back to the legacy client.
+ // (An error will occur when using the client if the experiment
+ // is enabled and the v1 client is nil).
+ v1 = nil
+ } else {
+ v1 = &client{src: s}
+ }
+
+ // Create the legacy client.
legacy, err := vulnc.NewClient([]string{src}, vulnc.Options{
HTTPCache: newCache(),
})
@@ -30,7 +49,7 @@
return nil, err
}
- return &Client{legacy: &legacyClient{legacy}}, nil
+ return &Client{legacy: &legacyClient{legacy}, v1: v1}, nil
}
type PackageRequest struct {
@@ -81,16 +100,25 @@
return cli.IDs(ctx)
}
-// cli returns the underlying client, favoring the legacy client
-// if both are present.
-func (c *Client) cli(ctx context.Context) (cli, error) {
- if c.legacy != nil {
- return c.legacy, nil
- }
- if c.v1 != nil {
+// cli returns the underlying client.
+// If the v1 experiment is active, it attempts to reurn the v1 client,
+// falling back on the legacy client if not set.
+// Otherwise, it always returns the legacy client.
+func (c *Client) cli(ctx context.Context) (_ cli, err error) {
+ derrors.Wrap(&err, "Client.cli()")
+
+ if experiment.IsActive(ctx, internal.ExperimentVulndbV1) {
+ if c.v1 == nil {
+ return nil, fmt.Errorf("v1 experiment is set, but v1 client is nil")
+ }
return c.v1, nil
}
- return nil, fmt.Errorf("vuln.Client: no underlying client defined")
+
+ if c.legacy == nil {
+ return nil, fmt.Errorf("legacy vulndb client is nil")
+ }
+
+ return c.legacy, nil
}
// cli is an interface used temporarily to allow us to support
diff --git a/internal/vuln/client_test.go b/internal/vuln/client_test.go
index f521713..f58bba8 100644
--- a/internal/vuln/client_test.go
+++ b/internal/vuln/client_test.go
@@ -11,6 +11,8 @@
"testing"
"time"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/vuln/osv"
)
@@ -336,10 +338,9 @@
})
}
-// Test that Client can pick the right underlying client.
+// Test that Client can pick the right underlying client, based
+// on whether the v1 experiment is active.
func TestCli(t *testing.T) {
- ctx := context.Background()
-
v1, err := newTestV1Client(dbTxtar)
if err != nil {
t.Fatal(err)
@@ -347,8 +348,10 @@
legacy := newTestLegacyClient([]*osv.Entry{&testOSV1, &testOSV2, &testOSV3})
- t.Run("legacy preferred", func(t *testing.T) {
+ t.Run("legacy preferred if experiment inactive", func(t *testing.T) {
+ ctx := context.Background()
c := Client{legacy: legacy, v1: v1}
+
cli, err := c.cli(ctx)
if err != nil {
t.Fatal(err)
@@ -358,19 +361,32 @@
}
})
- t.Run("v1 if no legacy", func(t *testing.T) {
- c := Client{v1: v1}
+ t.Run("v1 preferred if experiment active", func(t *testing.T) {
+ ctx := experiment.NewContext(context.Background(), internal.ExperimentVulndbV1)
+
+ c := Client{legacy: legacy, v1: v1}
cli, err := c.cli(ctx)
if err != nil {
t.Fatal(err)
}
if _, ok := cli.(*client); !ok {
- t.Errorf("Client.cli() = %s, want type *clientV1", cli)
+ t.Errorf("Client.cli() = %s, want type *client", cli)
}
})
- t.Run("error if both nil", func(t *testing.T) {
- c := Client{}
+ t.Run("error if legacy nil and experiment inactive", func(t *testing.T) {
+ ctx := context.Background()
+ c := Client{v1: v1}
+ cli, err := c.cli(ctx)
+ if err == nil {
+ t.Errorf("Client.cli() = %s, want error", cli)
+ }
+ })
+
+ t.Run("error if v1 nil and experiment active", func(t *testing.T) {
+ ctx := experiment.NewContext(context.Background(), internal.ExperimentVulndbV1)
+
+ c := Client{legacy: legacy}
cli, err := c.cli(ctx)
if err == nil {
t.Errorf("Client.cli() = %s, want error", cli)
diff --git a/internal/vuln/vulns_test.go b/internal/vuln/vulns_test.go
index 0eb8d38..41a9c1b 100644
--- a/internal/vuln/vulns_test.go
+++ b/internal/vuln/vulns_test.go
@@ -11,11 +11,12 @@
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/vuln/osv"
)
func TestVulnsForPackage(t *testing.T) {
- ctx := context.Background()
e := osv.Entry{
ID: "GO-1",
Affected: []osv.Affected{{
@@ -151,7 +152,7 @@
mod: "std", pkg: "net/http", version: "go1.20", want: nil,
},
}
- test := func(t *testing.T, c *Client) {
+ test := func(t *testing.T, ctx context.Context, c *Client) {
for _, tc := range testCases {
{
t.Run(tc.name, func(t *testing.T) {
@@ -164,11 +165,12 @@
}
}
t.Run("legacy", func(t *testing.T) {
- test(t, &Client{legacy: legacyClient})
+ test(t, context.Background(), &Client{legacy: legacyClient})
})
t.Run("v1", func(t *testing.T) {
- test(t, &Client{v1: v1Client})
+ ctx := experiment.NewContext(context.Background(), internal.ExperimentVulndbV1)
+ test(t, ctx, &Client{v1: v1Client})
})
}
