Skip to content

Lift service discovery client out of CyberArkClient #765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SgtCoDFish merged 2 commits into from
Jan 28, 2026
Merged

Lift service discovery client out of CyberArkClient #765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b08d214
lift service discovery client out of CyberArkClient
SgtCoDFish Jan 27, 2026
e9b255c
add log warnings to flaky tests, simplify skipping
SgtCoDFish Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
21 changes: 10 additions & 11 deletions internal/cyberark/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@wallrj-cyberark wallrj-cyberark Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check for nil serviceMap here, and return an error, to avoid a panic, or make the argument a value rather than a pointer.

identityAPI := serviceMap.Identity.API
if identityAPI == "" {
return nil, errors.New("service discovery returned an empty identity API")
Copy link
Member

@wallrj-cyberark wallrj-cyberark Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: This error still talks about "service discovery returned..."

Suggested change
return nil, errors.New("service discovery returned an empty identity API")
return nil, errors.New("missing or empty serviceMap.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")
Copy link
Member

@wallrj-cyberark wallrj-cyberark Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: This error still talks about "service discovery returned..."

Suggested change
return nil, errors.New("service discovery returned an empty discovery API")
return nil, errors.New("missing or empty serviceMap.DiscoveryContext.API")

}

identityClient := identity.New(httpClient, identityAPI, cfg.Subdomain)

err := identityClient.LoginUsernamePassword(ctx, cfg.Username, []byte(cfg.Secret))
if err != nil {
return nil, err
}
Comment on lines +63 to +68
Copy link
Contributor Author

@SgtCoDFish SgtCoDFish Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Moving the login down isn't strictly required in this PR, but I did it so that we check both the identity and discovery API availability first, before we try the login. This means if we didn't get a disco API endpoint from service discovery, we won't do a pointless login attempt.


return dataupload.New(httpClient, discoveryAPI, identityClient.AuthenticateRequest), nil
}
29 changes: 22 additions & 7 deletions internal/cyberark/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package cyberark_test

import (
"crypto/x509"
"errors"
"testing"

"github.com/jetstack/venafi-connection-lib/http_client"
Expand All @@ -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"

Expand All @@ -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)
}

Expand All @@ -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)
}
22 changes: 17 additions & 5 deletions pkg/client/client_cyberark.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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)
}
Expand Down