[pkgsite] cmd/frontend, internal/vuln: add vulndb v1 experiment
1 view
Skip to first unread message
Tatiana Bradley (Gerrit)
Mar 29, 2023, 10:40:03 AM3/29/23
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, kokoro, Julie Qiu, golang-co...@googlegroups.com
Tatiana Bradley submitted this change.
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: internal/vuln/client.go
Insertions: 16, Deletions: 11.
@@ -9,8 +9,8 @@
"fmt"
"golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/experiment"
- "golang.org/x/pkgsite/internal/log"
vulnc "golang.org/x/vuln/client"
"golang.org/x/vuln/osv"
)
@@ -22,25 +22,26 @@
// database, and will otherwise read from the legacy database.
type Client struct {
legacy *legacyClient
- v1 *clientV1
+ 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 *clientV1
+ 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.
- // (A non-fatal error will be logged when the client is called).
+ // (An error will occur when using the client if the experiment
+ // is enabled and the v1 client is nil).
v1 = nil
} else {
- v1 = &clientV1{src: s}
+ v1 = &client{src: s}
}
- // Create the lefacy client.
+ // Create the legacy client.
legacy, err := vulnc.NewClient([]string{src}, vulnc.Options{
HTTPCache: newCache(),
})
@@ -103,23 +104,27 @@
// 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) (client, error) {
+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 c.v1, nil
+ if c.v1 == nil {
+ return nil, fmt.Errorf("v1 experiment is set, but v1 client is nil")
}
- log.Errorf(ctx, "vuln.Client: v1 experiment is set, but v1 client is nil - falling back to legacy client")
+ return c.v1, nil
}
+
if c.legacy == nil {
- return nil, fmt.Errorf("vuln.Client: no valid underlying client")
+ return nil, fmt.Errorf("legacy vulndb client is nil")
}
+
return c.legacy, nil
}
-// client is an interface used temporarily to allow us to support
+// cli is an interface used temporarily to allow us to support
// both the legacy and v1 databases. It will be removed once we have
-// confidence that the v1 client is working.
-type client interface {
+// confidence that the v1 cli is working.
+type cli interface {
ByPackage(ctx context.Context, req *PackageRequest) (_ []*osv.Entry, err error)
ByID(ctx context.Context, id string) (*osv.Entry, error)
ByAlias(ctx context.Context, alias string) ([]*osv.Entry, error)
```
```
The name of the file: internal/vuln/client_test.go
Insertions: 14, Deletions: 3.
@@ -113,7 +113,7 @@
)
func TestByPackage(t *testing.T) {
- runClientTest(t, func(t *testing.T, c client) {
+ runClientTest(t, func(t *testing.T, c cli) {
tests := []struct {
name string
req *PackageRequest
@@ -207,6 +207,22 @@
},
want: nil,
},
+ {
+ name: "go prefix, no patch version - in range",
+ req: &PackageRequest{
+ Module: "stdlib",
+ Version: "go1.2",
+ },
+ want: []*osv.Entry{&testOSV1},
+ },
+ {
+ name: "go prefix, no patch version - out of range",
+ req: &PackageRequest{
+ Module: "stdlib",
+ Version: "go1.3",
+ },
+ want: nil,
+ },
}
for _, test := range tests {
@@ -233,7 +249,7 @@
}
func TestByAlias(t *testing.T) {
- runClientTest(t, func(t *testing.T, c client) {
+ runClientTest(t, func(t *testing.T, c cli) {
tests := []struct {
name string
alias string
@@ -272,7 +288,7 @@
}
func TestByID(t *testing.T) {
- runClientTest(t, func(t *testing.T, c client) {
+ runClientTest(t, func(t *testing.T, c cli) {
tests := []struct {
id string
want *osv.Entry
@@ -307,7 +323,7 @@
}
func TestIDs(t *testing.T) {
- runClientTest(t, func(t *testing.T, c client) {
+ runClientTest(t, func(t *testing.T, c cli) {
ctx := context.Background()
got, err := c.IDs(ctx)
@@ -322,7 +338,8 @@
})
}
-// 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) {
v1, err := newTestV1Client(dbTxtar)
if err != nil {
@@ -352,8 +369,8 @@
if err != nil {
t.Fatal(err)
}
- if _, ok := cli.(*clientV1); !ok {
- t.Errorf("Client.cli() = %s, want type *clientV1", cli)
+ if _, ok := cli.(*client); !ok {
+ t.Errorf("Client.cli() = %s, want type *client", cli)
}
})
@@ -365,10 +382,20 @@
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)
+ }
+ })
}
// Run the test for both the v1 and legacy clients.
-func runClientTest(t *testing.T, test func(*testing.T, client)) {
+func runClientTest(t *testing.T, test func(*testing.T, cli)) {
v1, err := newTestV1Client(dbTxtar)
if err != nil {
t.Fatal(err)
```
Approvals:
Julie Qiu: Looks good to me, approved
Tatiana Bradley: Looks good to me, but someone else must approve; Run TryBots
kokoro: TryBots succeeded
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 <tatiana...@google.com>
TryBot-Result: kokoro <noreply...@google.com>
Run-TryBot: Tatiana Bradley <tatiana...@google.com>
Reviewed-by: Julie Qiu <juli...@google.com>
---
M internal/experiment.go
M internal/vuln/client.go
M internal/vuln/client_test.go
M internal/vuln/vulns_test.go
4 files changed, 74 insertions(+), 26 deletions(-)
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})
})
}
To view, visit change 476556. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages