bitbucket: Initialise using synced rate limiters (#33773)

Instead of setting default ones in the package. This is consistent with
how all the other code hosts treat rate limit initialisation.

enterprise/internal/authz/bitbucketserver/authz.go

@@ -45,7 +45,7 @@ func newAuthzProvider(
 
 	var errs error
 
-	cli, err := bitbucketserver.NewClient(c.BitbucketServerConnection, nil)
+	cli, err := bitbucketserver.NewClient(c.URN, c.BitbucketServerConnection, nil)
 	if err != nil {
 		errs = errors.Append(errs, err)
 		return nil, errs

enterprise/internal/batches/sources/bitbucketserver.go

@@ -31,10 +31,7 @@ func NewBitbucketServerSource(svc *types.ExternalService, cf *httpcli.Factory) (
 	if err := jsonc.Unmarshal(svc.Config, &c); err != nil {
 		return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err)
 	}
-	return newBitbucketServerSource(&c, cf, nil)
-}
 
-func newBitbucketServerSource(c *schema.BitbucketServerConnection, cf *httpcli.Factory, au auth.Authenticator) (*BitbucketServerSource, error) {
 	if cf == nil {
 		cf = httpcli.ExternalClientFactory
 	}
@@ -46,15 +43,11 @@ func newBitbucketServerSource(c *schema.BitbucketServerConnection, cf *httpcli.F
 		return nil, err
 	}
 
-	client, err := bitbucketserver.NewClient(c, cli)
+	client, err := bitbucketserver.NewClient(svc.URN(), &c, cli)
 	if err != nil {
 		return nil, err
 	}
 
-	if au != nil {
-		client = client.WithAuthenticator(au)
-	}
-
 	return &BitbucketServerSource{
 		au:     client.Auth,
 		client: client,

internal/extsvc/bitbucketcloud/client.go

@@ -28,21 +28,6 @@ import (
 // The metric generated here will be named as "src_bitbucket_cloud_requests_total".
 var requestCounter = metrics.NewRequestMeter("bitbucket_cloud", "Total number of requests sent to the Bitbucket Cloud API.")
 
-// These fields define the self-imposed Bitbucket rate limit (since Bitbucket Cloud does
-// not have a concept of rate limiting in HTTP response headers).
-//
-// See https://godoc.org/golang.org/x/time/rate#Limiter for an explanation of these fields.
-//
-// The limits chosen here are based on the following logic: Bitbucket Cloud restricts
-// "List all repositories" requests (which are a good portion of our requests) to 1,000/hr,
-// and they restrict "List a user or team's repositories" requests (which are roughly equal
-// to our repository lookup requests) to 1,000/hr.
-// See `pkg/extsvc/bitbucketserver/client.go` for the calculations behind these limits`
-const (
-	rateLimitRequestsPerSecond = 2 // 120/min or 7200/hr
-	rateLimitMaxBurstRequests  = 500
-)
-
 // Client access a Bitbucket Cloud via the REST API 2.0.
 type Client struct {
 	// HTTP Client used to communicate with the API
@@ -57,13 +42,13 @@ type Client struct {
 
 	// RateLimit is the self-imposed rate limiter (since Bitbucket does not have a concept
 	// of rate limiting in HTTP response headers).
-	RateLimit *rate.Limiter
+	rateLimit *rate.Limiter
 }
 
 // NewClient creates a new Bitbucket Cloud API client from the given external
 // service configuration. If a nil httpClient is provided, http.DefaultClient
 // will be used.
-func NewClient(config *schema.BitbucketCloudConnection, httpClient httpcli.Doer) (*Client, error) {
+func NewClient(urn string, config *schema.BitbucketCloudConnection, httpClient httpcli.Doer) (*Client, error) {
 	if httpClient == nil {
 		httpClient = httpcli.ExternalDoer
 	}
@@ -83,20 +68,15 @@ func NewClient(config *schema.BitbucketCloudConnection, httpClient httpcli.Doer)
 		return nil, err
 	}
 
-	// Normally our registry will return a default infinite limiter when nothing has been
-	// synced from config. However, we always want to ensure there is at least some form of rate
-	// limiting for Bitbucket.
-	defaultLimiter := rate.NewLimiter(rateLimitRequestsPerSecond, rateLimitMaxBurstRequests)
-	l := ratelimit.DefaultRegistry.GetOrSet(apiURL.String(), defaultLimiter)
-
 	return &Client{
 		httpClient: httpClient,
 		URL:        extsvc.NormalizeBaseURL(apiURL),
 		Auth: &auth.BasicAuth{
 			Username: config.Username,
 			Password: config.AppPassword,
 		},
-		RateLimit: l,
+		// Default limits are defined in extsvc.GetLimitFromConfig
+		rateLimit: ratelimit.DefaultRegistry.Get(urn),
 	}, nil
 }
 
@@ -112,7 +92,7 @@ func (c *Client) WithAuthenticator(a auth.Authenticator) *Client {
 		httpClient: c.httpClient,
 		URL:        c.URL,
 		Auth:       a,
-		RateLimit:  c.RateLimit,
+		rateLimit:  c.rateLimit,
 	}
 }
 
@@ -189,7 +169,7 @@ func (c *Client) do(ctx context.Context, req *http.Request, result interface{})
 	}
 
 	startWait := time.Now()
-	if err := c.RateLimit.Wait(ctx); err != nil {
+	if err := c.rateLimit.Wait(ctx); err != nil {
 		return err
 	}
 

internal/extsvc/bitbucketcloud/testing.go

@@ -35,7 +35,7 @@ func NewTestClient(t testing.TB, name string, update bool) (*Client, func()) {
 		t.Fatal(err)
 	}
 
-	cli, err := NewClient(&schema.BitbucketCloudConnection{
+	cli, err := NewClient("urn", &schema.BitbucketCloudConnection{
 		ApiURL:      "https://api.bitbucket.org",
 		Username:    GetenvTestBitbucketCloudUsername(),
 		AppPassword: os.Getenv("BITBUCKET_CLOUD_APP_PASSWORD"),

internal/extsvc/bitbucketserver/client.go

@@ -36,34 +36,8 @@ import (
 // The metric generated here will be named as "src_bitbucket_requests_total".
 var requestCounter = metrics.NewRequestMeter("bitbucket", "Total number of requests sent to the Bitbucket API.")
 
-// These fields define the self-imposed Bitbucket rate limit (since Bitbucket Server does
-// not have a concept of rate limiting in HTTP response headers).
-//
-// See https://godoc.org/golang.org/x/time/rate#Limiter for an explanation of these fields.
-//
-// We chose the limits here based on the fact that Sourcegraph is a heavy consumer of the Bitbucket
-// Server API and that a large customer had reported to us their Bitbucket instance receives
-// ~100 req/s so it seems reasonable for us to (at max) consume ~8 req/s.
-//
-// Note that, for comparison, Bitbucket Cloud restricts "List all repositories" requests (which are
-// a good portion of our requests) to 1,000/hr, and they restrict "List a user or team's repositories"
-// requests (which are roughly equal to our repository lookup requests) to 1,000/hr. We perform a list
-// repositories request for every 1000 repositories on Bitbucket every 1m by default, so for someone
-// with 20,000 Bitbucket repositories we need 20,000/1000 requests per minute (1200/hr) + overhead for
-// repository lookup requests by users, and requests for identifying which repositories a user has
-// access to (if authorization is in use) and requests for changeset synchronization if it is in use.
-//
-// These are our default values, they can be changed in configuration
-const (
-	defaultRateLimit      = rate.Limit(8) // 480/min or 28,800/hr
-	defaultRateLimitBurst = 500
-)
-
 // Client access a Bitbucket Server via the REST API.
 type Client struct {
-	// HTTP Client used to communicate with the API
-	httpClient httpcli.Doer
-
 	// URL is the base URL of Bitbucket Server.
 	URL *url.URL
 
@@ -78,16 +52,20 @@ type Client struct {
 	//   This is generally set using SetOAuth.
 	Auth auth.Authenticator
 
-	// RateLimit is the self-imposed rate limiter (since Bitbucket does not have a concept
-	// of rate limiting in HTTP response headers).
-	RateLimit *rate.Limiter
+	// HTTP Client used to communicate with the API
+	httpClient httpcli.Doer
+
+	// RateLimit is the self-imposed rate limiter (since Bitbucket does not have a
+	// concept of rate limiting in HTTP response headers). Default limits are defined
+	// in extsvc.GetLimitFromConfig
+	rateLimit *rate.Limiter
 }
 
 // NewClient returns an authenticated Bitbucket Server API client with
 // the provided configuration. If a nil httpClient is provided, http.DefaultClient
 // will be used.
-func NewClient(config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) {
-	client, err := newClient(config, httpClient)
+func NewClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) {
+	client, err := newClient(urn, config, httpClient)
 	if err != nil {
 		return nil, err
 	}
@@ -114,7 +92,7 @@ func NewClient(config *schema.BitbucketServerConnection, httpClient httpcli.Doer
 	return client, nil
 }
 
-func newClient(config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) {
+func newClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) {
 	u, err := url.Parse(config.Url)
 	if err != nil {
 		return nil, err
@@ -125,16 +103,11 @@ func newClient(config *schema.BitbucketServerConnection, httpClient httpcli.Doer
 	}
 	httpClient = requestCounter.Doer(httpClient, categorize)
 
-	// Normally our registry will return a default infinite limiter when nothing has been
-	// synced from config. However, we always want to ensure there is at least some form of rate
-	// limiting for Bitbucket.
-	defaultLimiter := rate.NewLimiter(defaultRateLimit, defaultRateLimitBurst)
-	l := ratelimit.DefaultRegistry.GetOrSet(u.String(), defaultLimiter)
-
 	return &Client{
 		httpClient: httpClient,
 		URL:        u,
-		RateLimit:  l,
+		// Default limits are defined in extsvc.GetLimitFromConfig
+		rateLimit: ratelimit.DefaultRegistry.Get(urn),
 	}, nil
 }
 
@@ -145,7 +118,7 @@ func (c *Client) WithAuthenticator(a auth.Authenticator) *Client {
 	return &Client{
 		httpClient: c.httpClient,
 		URL:        c.URL,
-		RateLimit:  c.RateLimit,
+		rateLimit:  c.rateLimit,
 		Auth:       a,
 	}
 }
@@ -967,7 +940,7 @@ func (c *Client) do(ctx context.Context, req *http.Request, result interface{})
 	}
 
 	startWait := time.Now()
-	if err := c.RateLimit.Wait(ctx); err != nil {
+	if err := c.rateLimit.Wait(ctx); err != nil {
 		return nil, err
 	}
 

internal/extsvc/bitbucketserver/client_test.go

@@ -1090,7 +1090,7 @@ func TestAuth(t *testing.T) {
 		// Ensure that the different configuration types create the right
 		// implicit Authenticator.
 		t.Run("bearer token", func(t *testing.T) {
-			client, err := NewClient(&schema.BitbucketServerConnection{
+			client, err := NewClient("urn", &schema.BitbucketServerConnection{
 				Url:   "http://example.com/",
 				Token: "foo",
 			}, nil)
@@ -1107,7 +1107,7 @@ func TestAuth(t *testing.T) {
 		})
 
 		t.Run("basic auth", func(t *testing.T) {
-			client, err := NewClient(&schema.BitbucketServerConnection{
+			client, err := NewClient("urn", &schema.BitbucketServerConnection{
 				Url:      "http://example.com/",
 				Username: "foo",
 				Password: "bar",
@@ -1125,7 +1125,7 @@ func TestAuth(t *testing.T) {
 		})
 
 		t.Run("OAuth 1 error", func(t *testing.T) {
-			if _, err := NewClient(&schema.BitbucketServerConnection{
+			if _, err := NewClient("urn", &schema.BitbucketServerConnection{
 				Url: "http://example.com/",
 				Authorization: &schema.BitbucketServerAuthorization{
 					Oauth: schema.BitbucketServerOAuth{
@@ -1150,7 +1150,7 @@ func TestAuth(t *testing.T) {
 			pemKey := pem.EncodeToMemory(&pem.Block{Bytes: block})
 			signingKey := base64.StdEncoding.EncodeToString(pemKey)
 
-			client, err := NewClient(&schema.BitbucketServerConnection{
+			client, err := NewClient("urn", &schema.BitbucketServerConnection{
 				Url: "http://example.com/",
 				Authorization: &schema.BitbucketServerAuthorization{
 					Oauth: schema.BitbucketServerOAuth{
@@ -1229,7 +1229,7 @@ func TestClient_WithAuthenticator(t *testing.T) {
 
 	old := &Client{
 		URL:       uri,
-		RateLimit: rate.NewLimiter(defaultRateLimit, defaultRateLimitBurst),
+		rateLimit: rate.NewLimiter(10, 10),
 		Auth:      &auth.BasicAuth{Username: "johnsson", Password: "mothersmaidenname"},
 	}
 
@@ -1247,8 +1247,8 @@ func TestClient_WithAuthenticator(t *testing.T) {
 		t.Fatalf("url: want %q but got %q", old.URL, newClient.URL)
 	}
 
-	if newClient.RateLimit != old.RateLimit {
-		t.Fatalf("RateLimit: want %#v but got %#v", old.RateLimit, newClient.RateLimit)
+	if newClient.rateLimit != old.rateLimit {
+		t.Fatalf("RateLimit: want %#v but got %#v", old.rateLimit, newClient.rateLimit)
 	}
 }
 

internal/extsvc/bitbucketserver/testing.go

@@ -42,7 +42,7 @@ func NewTestClient(t testing.TB, name string, update bool) (*Client, func()) {
 		Url:   instanceURL,
 	}
 
-	cli, err := NewClient(c, hc)
+	cli, err := NewClient("urn", c, hc)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/extsvc/types.go

@@ -136,8 +136,6 @@ const (
 
 	// TypeOther is the (api.ExternalRepoSpec).ServiceType value for other projects.
 	TypeOther = "other"
-
-	defaultRateLimit = rate.Limit(2)
 )
 
 // KindToType returns a Type constants given a Kind
@@ -417,7 +415,7 @@ func GetLimitFromConfig(kind string, config interface{}) (rate.Limit, error) {
 			limit = limitOrInf(c.RateLimit.Enabled, c.RateLimit.RequestsPerHour)
 		}
 	case *schema.BitbucketCloudConnection:
-		limit = defaultRateLimit
+		limit = rate.Limit(2)
 		if c != nil && c.RateLimit != nil {
 			limit = limitOrInf(c.RateLimit.Enabled, c.RateLimit.RequestsPerHour)
 		}
@@ -427,7 +425,7 @@ func GetLimitFromConfig(kind string, config interface{}) (rate.Limit, error) {
 			limit = limitOrInf(c.RateLimit.Enabled, c.RateLimit.RequestsPerHour)
 		}
 	case *schema.JVMPackagesConnection:
-		limit = defaultRateLimit
+		limit = rate.Limit(2)
 		if c != nil && c.Maven.RateLimit != nil {
 			limit = limitOrInf(c.Maven.RateLimit.Enabled, c.Maven.RateLimit.RequestsPerHour)
 		}

internal/repos/bitbucketcloud.go

@@ -58,7 +58,7 @@ func newBitbucketCloudSource(svc *types.ExternalService, c *schema.BitbucketClou
 		return nil, err
 	}
 
-	client, err := bitbucketcloud.NewClient(c, cli)
+	client, err := bitbucketcloud.NewClient(svc.URN(), c, cli)
 	if err != nil {
 		return nil, err
 	}

internal/repos/bitbucketserver.go

@@ -70,7 +70,7 @@ func newBitbucketServerSource(svc *types.ExternalService, c *schema.BitbucketSer
 		return nil, err
 	}
 
-	client, err := bitbucketserver.NewClient(c, cli)
+	client, err := bitbucketserver.NewClient(svc.URN(), c, cli)
 	if err != nil {
 		return nil, err
 	}