Merge pull request #700 from jetstack/VC-43403-dataupload-client

[VC-43403] Refactor the CyberArk dataupload client to take an HTTP client

Author: wallrj-cyberark

pkg/client/client_cyberark.go

@@ -6,4 +6,4 @@ import (
 
 type CyberArkClient = dataupload.CyberArkClient
 
-var NewCyberArkClient = dataupload.NewCyberArkClient
+var NewCyberArkClient = dataupload.New

pkg/internal/cyberark/dataupload/dataupload.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
-	"crypto/x509"
 	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
@@ -13,8 +12,6 @@ import (
 	"net/http"
 	"net/url"
 
-	"k8s.io/client-go/transport"
-
 	"github.com/jetstack/preflight/api"
 	"github.com/jetstack/preflight/pkg/version"
 )
@@ -31,8 +28,8 @@ const (
 )
 
 type CyberArkClient struct {
-	baseURL string
-	client  *http.Client
+	baseURL    string
+	httpClient *http.Client
 
 	authenticateRequest func(req *http.Request) error
 }
@@ -41,19 +38,12 @@ type Options struct {
 	ClusterName string
 }
 
-func NewCyberArkClient(trustedCAs *x509.CertPool, baseURL string, authenticateRequest func(req *http.Request) error) (*CyberArkClient, error) {
-	cyberClient := &http.Client{}
-	tr := http.DefaultTransport.(*http.Transport).Clone()
-	if trustedCAs != nil {
-		tr.TLSClientConfig.RootCAs = trustedCAs
-	}
-	cyberClient.Transport = transport.NewDebuggingRoundTripper(tr, transport.DebugByContext)
-
+func New(httpClient *http.Client, baseURL string, authenticateRequest func(req *http.Request) error) *CyberArkClient {
 	return &CyberArkClient{
 		baseURL:             baseURL,
-		client:              cyberClient,
+		httpClient:          httpClient,
 		authenticateRequest: authenticateRequest,
-	}, nil
+	}
 }
 
 // PostDataReadingsWithOptions PUTs the supplied payload to an [AWS presigned URL] which it obtains via the CyberArk inventory API.
@@ -96,7 +86,7 @@ func (c *CyberArkClient) PostDataReadingsWithOptions(ctx context.Context, payloa
 	req.Header.Set("X-Amz-Checksum-Sha256", checksumBase64)
 	version.SetUserAgent(req)
 
-	res, err := c.client.Do(req)
+	res, err := c.httpClient.Do(req)
 	if err != nil {
 		return err
 	}
@@ -145,7 +135,7 @@ func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksu
 	}
 	version.SetUserAgent(req)
 
-	res, err := c.client.Do(req)
+	res, err := c.httpClient.Do(req)
 	if err != nil {
 		return "", err
 	}

pkg/internal/cyberark/dataupload/dataupload_test.go

@@ -2,7 +2,6 @@ package dataupload_test
 
 import (
 	"crypto/x509"
-	"encoding/pem"
 	"fmt"
 	"net/http"
 	"os"
@@ -109,19 +108,14 @@ func TestCyberArkClient_PostDataReadingsWithOptions(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			server := dataupload.MockDataUploadServer()
-			defer server.Close()
+			logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+			ctx := klog.NewContext(t.Context(), logger)
 
-			certPool := x509.NewCertPool()
-			require.True(t, certPool.AppendCertsFromPEM(pem.EncodeToMemory(&pem.Block{
-				Type:  "CERTIFICATE",
-				Bytes: server.Server.TLS.Certificates[0].Certificate[0],
-			})))
+			datauploadAPIBaseURL, httpClient := dataupload.MockDataUploadServer(t)
 
-			cyberArkClient, err := dataupload.NewCyberArkClient(certPool, server.Server.URL, tc.authenticate)
-			require.NoError(t, err)
+			cyberArkClient := dataupload.New(httpClient, datauploadAPIBaseURL, tc.authenticate)
 
-			err = cyberArkClient.PostDataReadingsWithOptions(t.Context(), tc.payload, tc.opts)
+			err := cyberArkClient.PostDataReadingsWithOptions(ctx, tc.payload, tc.opts)
 			tc.requireFn(t, err)
 		})
 	}
@@ -153,12 +147,8 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) {
 	logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
 	ctx := klog.NewContext(t.Context(), logger)
 
-	const (
-		discoveryContextServiceName = "inventory"
-		separator                   = "."
-	)
-
-	serviceURL := fmt.Sprintf("https://%s%s%s.%s", subdomain, separator, discoveryContextServiceName, platformDomain)
+	// TODO(wallrj): get this from the servicediscovery API instead.
+	inventoryAPI := fmt.Sprintf("https://%s.inventory.%s", subdomain, platformDomain)
 
 	var rootCAs *x509.CertPool
 	httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs)
@@ -173,9 +163,7 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) {
 	err = identityClient.LoginUsernamePassword(ctx, username, []byte(secret))
 	require.NoError(t, err)
 
-	cyberArkClient, err := dataupload.NewCyberArkClient(nil, serviceURL, identityClient.AuthenticateRequest)
-	require.NoError(t, err)
-
+	cyberArkClient := dataupload.New(httpClient, inventoryAPI, identityClient.AuthenticateRequest)
 	err = cyberArkClient.PostDataReadingsWithOptions(ctx, api.DataReadingsPost{}, dataupload.Options{
 		ClusterName: "bb068932-c80d-460d-88df-34bc7f3f3297",
 	})

pkg/internal/cyberark/dataupload/mock.go

@@ -8,10 +8,11 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"testing"
 
-	"github.com/jetstack/preflight/pkg/version"
+	"k8s.io/client-go/transport"
 
-	_ "embed"
+	"github.com/jetstack/preflight/pkg/version"
 )
 
 const (
@@ -21,21 +22,39 @@ const (
 )
 
 type mockDataUploadServer struct {
-	Server *httptest.Server
+	t         testing.TB
+	serverURL string
 }
 
-// MockDataUploadServer returns a mocked data upload server with default values.
-func MockDataUploadServer() *mockDataUploadServer {
-	mds := &mockDataUploadServer{}
-	mds.Server = httptest.NewTLSServer(mds)
-	return mds
-}
-
-func (mds *mockDataUploadServer) Close() {
-	mds.Server.Close()
+// MockDataUploadServer starts a server which mocks the CyberArk
+// Discovery and Context API, and an HTTP client with the CA certs needed to
+// connect to it.
+//
+// The returned URL can be supplied to the `dataupload.New` function as the base
+// URL for the discoverycontext API.
+//
+// The returned HTTP client has a transport which logs requests and responses
+// depending on log level of the logger supplied in the context.
+//
+// The mock server will return a successful response when the cluster ID matches
+// successClusterID. Other cluster IDs can be used to trigger various failure
+// responses.
+func MockDataUploadServer(t testing.TB) (string, *http.Client) {
+	mux := http.NewServeMux()
+	server := httptest.NewTLSServer(mux)
+	t.Cleanup(server.Close)
+	mds := &mockDataUploadServer{
+		t:         t,
+		serverURL: server.URL,
+	}
+	mux.Handle("/", mds)
+	httpClient := server.Client()
+	httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext)
+	return server.URL, httpClient
 }
 
 func (mds *mockDataUploadServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	mds.t.Log(r.Method, r.RequestURI)
 	switch r.URL.Path {
 	case apiPathSnapshotLinks:
 		mds.handleSnapshotLinks(w, r)
@@ -109,7 +128,7 @@ func (mds *mockDataUploadServer) handleSnapshotLinks(w http.ResponseWriter, r *h
 	// Write response body
 	w.WriteHeader(http.StatusOK)
 	w.Header().Set("Content-Type", "application/json")
-	presignedURL := mds.Server.URL + "/presigned-upload"
+	presignedURL := mds.serverURL + "/presigned-upload"
 	_ = json.NewEncoder(w).Encode(struct {
 		URL string `json:"url"`
 	}{presignedURL})