From 3fa015d013ec39e60ffb408cf5f4ea1c6eb5873d Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 27 Aug 2025 17:40:54 +0100 Subject: [PATCH] CyberArk(dataupload): simplify client initialization and improve testability - Replaced `NewCyberArkClient` with `New` for a cleaner API - Removed dependency on `x509.CertPool` and `transport.NewDebuggingRoundTripper` - Updated `CyberArkClient` to use `http.Client` directly - Refactored tests to use `MockDataUploadServer` with `testing.TB` for better cleanup - Simplified mock server setup and logging for improved maintainability Signed-off-by: Richard Wall --- pkg/client/client_cyberark.go | 2 +- .../cyberark/dataupload/dataupload.go | 24 +++------- .../cyberark/dataupload/dataupload_test.go | 28 ++++-------- pkg/internal/cyberark/dataupload/mock.go | 45 +++++++++++++------ 4 files changed, 48 insertions(+), 51 deletions(-) diff --git a/pkg/client/client_cyberark.go b/pkg/client/client_cyberark.go index af26e96c..c5800076 100644 --- a/pkg/client/client_cyberark.go +++ b/pkg/client/client_cyberark.go @@ -6,4 +6,4 @@ import ( type CyberArkClient = dataupload.CyberArkClient -var NewCyberArkClient = dataupload.NewCyberArkClient +var NewCyberArkClient = dataupload.New diff --git a/pkg/internal/cyberark/dataupload/dataupload.go b/pkg/internal/cyberark/dataupload/dataupload.go index e2b8a868..86997c18 100644 --- a/pkg/internal/cyberark/dataupload/dataupload.go +++ b/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 } diff --git a/pkg/internal/cyberark/dataupload/dataupload_test.go b/pkg/internal/cyberark/dataupload/dataupload_test.go index 523c2c9b..f453879e 100644 --- a/pkg/internal/cyberark/dataupload/dataupload_test.go +++ b/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", }) diff --git a/pkg/internal/cyberark/dataupload/mock.go b/pkg/internal/cyberark/dataupload/mock.go index ad92e462..dd84ebac 100644 --- a/pkg/internal/cyberark/dataupload/mock.go +++ b/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})