Merge pull request #765 from jetstack/lift-sd

Lift service discovery client out of CyberArkClient

Author: SgtCoDFish

cmd/agent_test.go

@@ -33,6 +33,9 @@ func TestOutputModes(t *testing.T) {
 
 	t.Run("machinehub", func(t *testing.T) {
 		arktesting.SkipIfNoEnv(t)
+
+		t.Log("This test runs against a live service and has been known to flake. If you see timeout issues it's possible that the test is flaking and it could be unrelated to your changes.")
+
 		runSubprocess(t, repoRoot, []string{
 			"--agent-config-file", filepath.Join(repoRoot, "examples/machinehub/config.yaml"),
 			"--input-path", filepath.Join(repoRoot, "examples/machinehub/input.json"),

internal/cyberark/client.go

@@ -49,24 +49,23 @@ func LoadClientConfigFromEnvironment() (ClientConfig, error) {
 // NewDatauploadClient initializes and returns a new CyberArk Data Upload client.
 // It performs service discovery to find the necessary API endpoints and authenticates
 // using the provided client configuration.
-func NewDatauploadClient(ctx context.Context, httpClient *http.Client, cfg ClientConfig) (*dataupload.CyberArkClient, error) {
-	discoveryClient := servicediscovery.New(httpClient)
-	serviceMap, err := discoveryClient.DiscoverServices(ctx, cfg.Subdomain)
-	if err != nil {
-		return nil, err
-	}
+func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMap *servicediscovery.Services, cfg ClientConfig) (*dataupload.CyberArkClient, error) {
 	identityAPI := serviceMap.Identity.API
 	if identityAPI == "" {
 		return nil, errors.New("service discovery returned an empty identity API")
 	}
-	identityClient := identity.New(httpClient, identityAPI, cfg.Subdomain)
-	err = identityClient.LoginUsernamePassword(ctx, cfg.Username, []byte(cfg.Secret))
-	if err != nil {
-		return nil, err
-	}
+
 	discoveryAPI := serviceMap.DiscoveryContext.API
 	if discoveryAPI == "" {
 		return nil, errors.New("service discovery returned an empty discovery API")
 	}
+
+	identityClient := identity.New(httpClient, identityAPI, cfg.Subdomain)
+
+	err := identityClient.LoginUsernamePassword(ctx, cfg.Username, []byte(cfg.Secret))
+	if err != nil {
+		return nil, err
+	}
+
 	return dataupload.New(httpClient, discoveryAPI, identityClient.AuthenticateRequest), nil
 }

internal/cyberark/client_test.go

@@ -2,7 +2,6 @@ package cyberark_test
 
 import (
 	"crypto/x509"
-	"errors"
 	"testing"
 
 	"github.com/jetstack/venafi-connection-lib/http_client"
@@ -13,6 +12,7 @@ import (
 	"github.com/jetstack/preflight/internal/cyberark"
 	"github.com/jetstack/preflight/internal/cyberark/dataupload"
 	"github.com/jetstack/preflight/internal/cyberark/servicediscovery"
+	arktesting "github.com/jetstack/preflight/internal/cyberark/testing"
 	"github.com/jetstack/preflight/pkg/testutil"
 	"github.com/jetstack/preflight/pkg/version"
 
@@ -32,13 +32,21 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {
 		Secret:    "somepassword",
 	}
 
-	cl, err := cyberark.NewDatauploadClient(ctx, httpClient, cfg)
+	discoveryClient := servicediscovery.New(httpClient)
+
+	serviceMap, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
+	if err != nil {
+		t.Fatalf("failed to discover mock services: %v", err)
+	}
+
+	cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, cfg)
 	require.NoError(t, err)
 
 	err = cl.PutSnapshot(ctx, dataupload.Snapshot{
 		ClusterID:    "ffffffff-ffff-ffff-ffff-ffffffffffff",
 		AgentVersion: version.PreflightVersion,
 	})
+
 	require.NoError(t, err)
 }
 
@@ -55,26 +63,33 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {
 //	go test ./internal/cyberark \
 //	  -v -count 1 -run TestCyberArkClient_PutSnapshot_RealAPI -args -testing.v 6
 func TestCyberArkClient_PutSnapshot_RealAPI(t *testing.T) {
+	arktesting.SkipIfNoEnv(t)
+
+	t.Log("This test runs against a live service and has been known to flake. If you see timeout issues it's possible that the test is flaking and it could be unrelated to your changes.")
+
 	logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
 	ctx := klog.NewContext(t.Context(), logger)
 
 	var rootCAs *x509.CertPool
 	httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs)
 
 	cfg, err := cyberark.LoadClientConfigFromEnvironment()
+	require.NoError(t, err)
+
+	discoveryClient := servicediscovery.New(httpClient)
+
+	serviceMap, err := discoveryClient.DiscoverServices(t.Context(), cfg.Subdomain)
 	if err != nil {
-		if errors.Is(err, cyberark.ErrMissingEnvironmentVariables) {
-			t.Skipf("Skipping: %s", err)
-		}
-		require.NoError(t, err)
+		t.Fatalf("failed to discover services: %v", err)
 	}
 
-	cl, err := cyberark.NewDatauploadClient(ctx, httpClient, cfg)
+	cl, err := cyberark.NewDatauploadClient(ctx, httpClient, serviceMap, cfg)
 	require.NoError(t, err)
 
 	err = cl.PutSnapshot(ctx, dataupload.Snapshot{
 		ClusterID:    "ffffffff-ffff-ffff-ffff-ffffffffffff",
 		AgentVersion: version.PreflightVersion,
 	})
+
 	require.NoError(t, err)
 }

pkg/client/client_cyberark.go

@@ -19,6 +19,7 @@ import (
 	"github.com/jetstack/preflight/api"
 	"github.com/jetstack/preflight/internal/cyberark"
 	"github.com/jetstack/preflight/internal/cyberark/dataupload"
+	"github.com/jetstack/preflight/internal/cyberark/servicediscovery"
 	"github.com/jetstack/preflight/pkg/logs"
 	"github.com/jetstack/preflight/pkg/version"
 )
@@ -37,10 +38,12 @@ var _ Client = &CyberArkClient{}
 // If the configuration is invalid or missing, an error is returned.
 func NewCyberArk(httpClient *http.Client) (*CyberArkClient, error) {
 	configLoader := cyberark.LoadClientConfigFromEnvironment
+
 	_, err := configLoader()
 	if err != nil {
 		return nil, err
 	}
+
 	return &CyberArkClient{
 		configLoader: configLoader,
 		httpClient:   httpClient,
@@ -56,6 +59,19 @@ func NewCyberArk(httpClient *http.Client) (*CyberArkClient, error) {
 // The supplied Options are not used by this publisher.
 func (o *CyberArkClient) PostDataReadingsWithOptions(ctx context.Context, readings []*api.DataReading, opts Options) error {
 	log := klog.FromContext(ctx)
+
+	cfg, err := o.configLoader()
+	if err != nil {
+		return fmt.Errorf("failed to load config: %w", err)
+	}
+
+	discoveryClient := servicediscovery.New(o.httpClient)
+
+	serviceMap, err := discoveryClient.DiscoverServices(ctx, cfg.Subdomain)
+	if err != nil {
+		return err
+	}
+
 	snapshot := baseSnapshotFromOptions(opts)
 
 	if err := convertDataReadings(defaultExtractorFunctions, readings, &snapshot); err != nil {
@@ -65,11 +81,7 @@ func (o *CyberArkClient) PostDataReadingsWithOptions(ctx context.Context, readin
 	// Minimize the snapshot to reduce size and improve privacy
 	minimizeSnapshot(log.V(logs.Debug), &snapshot)
 
-	cfg, err := o.configLoader()
-	if err != nil {
-		return err
-	}
-	datauploadClient, err := cyberark.NewDatauploadClient(ctx, o.httpClient, cfg)
+	datauploadClient, err := cyberark.NewDatauploadClient(ctx, o.httpClient, serviceMap, cfg)
 	if err != nil {
 		return fmt.Errorf("while initializing data upload client: %s", err)
 	}