Postman increase http client timeout (#4086)

The Postman source can encounter issues with reading response bodies due to context cancellation or HTTP client timeout while trying to read the body. This PR attempts to remedy this issue by:

1. Deferring the response bodies closing.
2. In the process of 1, I consolidated the read and close steps into the function that makes Postman API calls.
3. Assigning a timeout value to the client returned by RetryableHTTPClientTimeout.

An odd bug was found in the process of exploring RetryableHTTPClientTimeout. The client returned by RetryableHTTPClientTimeout with return httpClient.StandardClient() did not keep the timeout attribute assigned to the receiver httpClient earlier in the function. The client returned by the call to RetryableHTTPClientTimeout would have a Timeout value of 0, which then begs the question of how a http client could ever time out with a Timeout value of 0 in the first place.

Author: casey-tran

pkg/common/http.go

@@ -47,7 +47,7 @@ oyi3B43njTOQ5yOf+1CceWxG1bQVs5ZufpsMljq4Ui0/1lvh+wjChP4kqKOJ2qxq
 4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA
 mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57d
 emyPxgcYxn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=
------END CERTIFICATE-----	
+-----END CERTIFICATE-----
 `,
 	// 	CN = ISRG Root X2
 	// TODO: Expires September 17, 2040 at 9:00:00 AM Pacific Daylight Time
@@ -189,6 +189,7 @@ func RetryableHTTPClient(opts ...ClientOption) *http.Client {
 	return httpClient.StandardClient()
 }
 
+// RetryableHTTPClientTimeout returns a new http client with a specified timeout and RoundTripper transport
 func RetryableHTTPClientTimeout(timeOutSeconds int64, opts ...ClientOption) *http.Client {
 	httpClient := retryablehttp.NewClient()
 	httpClient.RetryMax = 3
@@ -199,7 +200,10 @@ func RetryableHTTPClientTimeout(timeOutSeconds int64, opts ...ClientOption) *htt
 	for _, opt := range opts {
 		opt(httpClient)
 	}
-	return httpClient.StandardClient()
+
+	standardClient := httpClient.StandardClient()
+	standardClient.Timeout = httpClient.HTTPClient.Timeout
+	return standardClient
 }
 
 const DefaultResponseTimeout = 5 * time.Second

pkg/common/http_test.go

@@ -240,3 +240,32 @@ func TestRetryableHTTPClientBackoff(t *testing.T) {
 		})
 	}
 }
+
+func TestRetryableHTTPClientTimeout(t *testing.T) {
+	testCases := []struct {
+		name            string
+		timeoutSeconds  int64
+		expectedTimeout time.Duration
+	}{
+		{
+			name:            "5 second timeout",
+			timeoutSeconds:  5,
+			expectedTimeout: 5 * time.Second,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			// Call the function with the test timeout value
+			client := RetryableHTTPClientTimeout(tc.timeoutSeconds)
+
+			// Verify that the timeout is set correctly
+			assert.Equal(t, tc.expectedTimeout, client.Timeout, "HTTP client timeout does not match expected value")
+
+			// Verify that the transport is a custom transport
+			_, isRoundTripperTransport := client.Transport.(*retryablehttp.RoundTripper)
+			assert.True(t, isRoundTripperTransport, "HTTP client transport is not a retryablehttp.RoundTripper")
+		})
+	}
+}

pkg/sources/postman/postman.go

@@ -116,7 +116,7 @@ func (s *Source) Init(ctx context.Context, name string, jobId sources.JobID, sou
 			return errors.New("Postman token is empty")
 		}
 		s.client = NewClient(conn.GetToken())
-		s.client.HTTPClient = common.RetryableHTTPClientTimeout(3)
+		s.client.HTTPClient = common.RetryableHTTPClientTimeout(10)
 		log.RedactGlobally(conn.GetToken())
 	case *sourcespb.Postman_Unauthenticated:
 		s.client = nil
@@ -752,8 +752,8 @@ func unpackWorkspace(workspacePath string) (Workspace, error) {
 		if err != nil {
 			return workspace, err
 		}
+		defer rc.Close()
 		contents, err := io.ReadAll(rc)
-		rc.Close()
 		if err != nil {
 			return workspace, err
 		}

pkg/sources/postman/postman_client.go

@@ -246,7 +246,8 @@ func checkResponseStatus(r *http.Response) error {
 	return fmt.Errorf("postman Request failed with status code: %d", r.StatusCode)
 }
 
-func (c *Client) getPostmanReq(ctx context.Context, url string, headers map[string]string) (*http.Response, error) {
+// getPostmanResponseBodyBytes makes a request to the Postman API and returns the response body as bytes.
+func (c *Client) getPostmanResponseBodyBytes(ctx context.Context, url string, headers map[string]string) ([]byte, error) {
 	req, err := c.NewRequest(url, headers)
 	if err != nil {
 		return nil, err
@@ -256,13 +257,19 @@ func (c *Client) getPostmanReq(ctx context.Context, url string, headers map[stri
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, fmt.Errorf("could not read postman response body: %w", err)
+	}
 
 	ctx.Logger().V(4).Info("postman api response headers are available", "response_header", resp.Header)
 
 	if err := checkResponseStatus(resp); err != nil {
 		return nil, err
 	}
-	return resp, nil
+	return body, nil
 }
 
 // EnumerateWorkspaces returns the workspaces for a given user (both private, public, team and personal).
@@ -276,17 +283,10 @@ func (c *Client) EnumerateWorkspaces(ctx context.Context) ([]Workspace, error) {
 	if err := c.WorkspaceAndCollectionRateLimiter.Wait(ctx); err != nil {
 		return nil, fmt.Errorf("could not wait for rate limiter during workspaces enumeration getting: %w", err)
 	}
-	r, err := c.getPostmanReq(ctx, "https://api.getpostman.com/workspaces", nil)
+	body, err := c.getPostmanResponseBodyBytes(ctx, "https://api.getpostman.com/workspaces", nil)
 	if err != nil {
-		return nil, fmt.Errorf("could not get workspaces during enumeration: %w", err)
+		return nil, fmt.Errorf("could not get postman workspace response bytes during enumeration: %w", err)
 	}
-
-	body, err := io.ReadAll(r.Body)
-	if err != nil {
-		return nil, fmt.Errorf("could not read response body for workspaces during enumeration: %w", err)
-	}
-	r.Body.Close()
-
 	if err := json.Unmarshal([]byte(body), &workspacesObj); err != nil {
 		return nil, fmt.Errorf("could not unmarshal workspaces JSON during enumeration: %w", err)
 	}
@@ -318,17 +318,10 @@ func (c *Client) GetWorkspace(ctx context.Context, workspaceUUID string) (Worksp
 	if err := c.WorkspaceAndCollectionRateLimiter.Wait(ctx); err != nil {
 		return Workspace{}, fmt.Errorf("could not wait for rate limiter during workspace getting: %w", err)
 	}
-	r, err := c.getPostmanReq(ctx, url, nil)
+	body, err := c.getPostmanResponseBodyBytes(ctx, url, nil)
 	if err != nil {
-		return Workspace{}, fmt.Errorf("could not get workspace (%s): %w", workspaceUUID, err)
+		return Workspace{}, fmt.Errorf("could not get postman workspace (%s) response bytes: %w", workspaceUUID, err)
 	}
-
-	body, err := io.ReadAll(r.Body)
-	if err != nil {
-		return Workspace{}, fmt.Errorf("could not read response body for workspace (%s): %w", workspaceUUID, err)
-	}
-	r.Body.Close()
-
 	if err := json.Unmarshal([]byte(body), &obj); err != nil {
 		return Workspace{}, fmt.Errorf("could not unmarshal workspace JSON for workspace (%s): %w", workspaceUUID, err)
 	}
@@ -346,16 +339,10 @@ func (c *Client) GetEnvironmentVariables(ctx context.Context, environment_uuid s
 	if err := c.GeneralRateLimiter.Wait(ctx); err != nil {
 		return VariableData{}, fmt.Errorf("could not wait for rate limiter during environment variable getting: %w", err)
 	}
-	r, err := c.getPostmanReq(ctx, url, nil)
-	if err != nil {
-		return VariableData{}, fmt.Errorf("could not get env variables for environment (%s): %w", environment_uuid, err)
-	}
-
-	body, err := io.ReadAll(r.Body)
+	body, err := c.getPostmanResponseBodyBytes(ctx, url, nil)
 	if err != nil {
-		return VariableData{}, fmt.Errorf("could not read env var response body for environment (%s): %w", environment_uuid, err)
+		return VariableData{}, fmt.Errorf("could not get postman environment (%s) response bytes: %w", environment_uuid, err)
 	}
-	r.Body.Close()
 	if err := json.Unmarshal([]byte(body), &obj); err != nil {
 		return VariableData{}, fmt.Errorf("could not unmarshal env variables JSON for environment (%s): %w", environment_uuid, err)
 	}
@@ -373,16 +360,10 @@ func (c *Client) GetCollection(ctx context.Context, collection_uuid string) (Col
 	if err := c.WorkspaceAndCollectionRateLimiter.Wait(ctx); err != nil {
 		return Collection{}, fmt.Errorf("could not wait for rate limiter during collection getting: %w", err)
 	}
-	r, err := c.getPostmanReq(ctx, url, nil)
-	if err != nil {
-		return Collection{}, fmt.Errorf("could not get collection (%s): %w", collection_uuid, err)
-	}
-
-	body, err := io.ReadAll(r.Body)
+	body, err := c.getPostmanResponseBodyBytes(ctx, url, nil)
 	if err != nil {
-		return Collection{}, fmt.Errorf("could not read response body for collection (%s): %w", collection_uuid, err)
+		return Collection{}, fmt.Errorf("could not get postman collection (%s) response bytes: %w", collection_uuid, err)
 	}
-	r.Body.Close()
 	if err := json.Unmarshal([]byte(body), &obj); err != nil {
 		return Collection{}, fmt.Errorf("could not unmarshal JSON for collection (%s): %w", collection_uuid, err)
 	}