Merge pull request #699 from jetstack/VC-43403-servicediscovery-client

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

Author: wallrj-cyberark

pkg/internal/cyberark/dataupload/dataupload_test.go

@@ -144,10 +144,6 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) {
 	subdomain := os.Getenv("ARK_SUBDOMAIN")
 	username := os.Getenv("ARK_USERNAME")
 	secret := os.Getenv("ARK_SECRET")
-	serviceDiscoveryAPI := os.Getenv("ARK_DISCOVERY_API")
-	if serviceDiscoveryAPI == "" {
-		serviceDiscoveryAPI = servicediscovery.ProdDiscoveryEndpoint
-	}
 
 	if platformDomain == "" || subdomain == "" || username == "" || secret == "" {
 		t.Skip("Skipping because one of the following environment variables is unset or empty: ARK_PLATFORM_DOMAIN, ARK_SUBDOMAIN, ARK_USERNAME, ARK_SECRET")
@@ -168,10 +164,7 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) {
 	httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs)
 	httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext)
 
-	discoveryClient := servicediscovery.New(
-		servicediscovery.WithHTTPClient(httpClient),
-		servicediscovery.WithCustomEndpoint(serviceDiscoveryAPI),
-	)
+	discoveryClient := servicediscovery.New(httpClient)
 
 	identityAPI, err := discoveryClient.DiscoverIdentityAPIURL(ctx, subdomain)
 	require.NoError(t, err)

pkg/internal/cyberark/identity/cmd/testidentity/main.go

@@ -24,10 +24,9 @@ import (
 // To test against a tenant on the integration platform, set:
 // ARK_DISCOVERY_API=https://platform-discovery.integration-cyberark.cloud/api/v2
 const (
-	subdomainFlag          = "subdomain"
-	usernameFlag           = "username"
-	passwordEnv            = "ARK_SECRET"
-	serviceDiscoveryAPIEnv = "ARK_DISCOVERY_API"
+	subdomainFlag = "subdomain"
+	usernameFlag  = "username"
+	passwordEnv   = "ARK_SECRET"
 )
 
 var (
@@ -49,19 +48,11 @@ func run(ctx context.Context) error {
 		return fmt.Errorf("no password provided in %s", passwordEnv)
 	}
 
-	serviceDiscoveryAPI := os.Getenv(serviceDiscoveryAPIEnv)
-	if serviceDiscoveryAPI == "" {
-		serviceDiscoveryAPI = servicediscovery.ProdDiscoveryEndpoint
-	}
-
 	var rootCAs *x509.CertPool
 	httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs)
 	httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext)
 
-	sdClient := servicediscovery.New(
-		servicediscovery.WithHTTPClient(httpClient),
-		servicediscovery.WithCustomEndpoint(serviceDiscoveryAPI),
-	)
+	sdClient := servicediscovery.New(httpClient)
 	identityAPI, err := sdClient.DiscoverIdentityAPIURL(ctx, subdomain)
 	if err != nil {
 		return fmt.Errorf("while performing service discovery: %s", err)

pkg/internal/cyberark/servicediscovery/discovery.go

@@ -7,15 +7,13 @@ import (
 	"io"
 	"net/http"
 	"net/url"
-	"time"
-
-	"k8s.io/client-go/transport"
+	"os"
 
 	"github.com/jetstack/preflight/pkg/version"
 )
 
 const (
-	ProdDiscoveryEndpoint = "https://platform-discovery.cyberark.cloud/api/v2/"
+	ProdDiscoveryAPIBaseURL = "https://platform-discovery.cyberark.cloud/api/v2/"
 
 	// identityServiceName is the name of the identity service we're looking for in responses from the Service Discovery API
 	// We were told to use the identity_administration field, not the identity_user_portal field.
@@ -30,40 +28,19 @@ const (
 // users to fetch URLs for various APIs available in CyberArk. This client is specialised to
 // fetch only API endpoints, since only API endpoints are required by the Venafi Kubernetes Agent currently.
 type Client struct {
-	client   *http.Client
-	endpoint string
-}
-
-// ClientOpt allows configuration of a Client when using New
-type ClientOpt func(*Client)
-
-// WithHTTPClient allows the user to specify a custom HTTP client for the discovery client
-func WithHTTPClient(httpClient *http.Client) ClientOpt {
-	return func(c *Client) {
-		c.client = httpClient
-	}
-}
-
-// WithCustomEndpoint sets the endpoint to a custom URL without checking that the URL is a CyberArk Service Discovery
-// server.
-func WithCustomEndpoint(endpoint string) ClientOpt {
-	return func(c *Client) {
-		c.endpoint = endpoint
-	}
+	client  *http.Client
+	baseURL string
 }
 
 // New creates a new CyberArk Service Discovery client, configurable with ClientOpt
-func New(clientOpts ...ClientOpt) *Client {
-	client := &Client{
-		client: &http.Client{
-			Timeout:   10 * time.Second,
-			Transport: transport.NewDebuggingRoundTripper(http.DefaultTransport, transport.DebugByContext),
-		},
-		endpoint: ProdDiscoveryEndpoint,
+func New(httpClient *http.Client) *Client {
+	baseURL := os.Getenv("ARK_DISCOVERY_API")
+	if baseURL == "" {
+		baseURL = ProdDiscoveryAPIBaseURL
 	}
-
-	for _, opt := range clientOpts {
-		opt(client)
+	client := &Client{
+		client:  httpClient,
+		baseURL: baseURL,
 	}
 
 	return client
@@ -72,7 +49,7 @@ func New(clientOpts ...ClientOpt) *Client {
 // DiscoverIdentityAPIURL fetches from the service discovery service for a given subdomain
 // and parses the CyberArk Identity API URL.
 func (c *Client) DiscoverIdentityAPIURL(ctx context.Context, subdomain string) (string, error) {
-	endpoint, err := url.JoinPath(c.endpoint, "services", "subdomain", subdomain)
+	endpoint, err := url.JoinPath(c.baseURL, "services", "subdomain", subdomain)
 	if err != nil {
 		return "", fmt.Errorf("failed to build a valid URL for subdomain %s; possibly an invalid endpoint: %s", subdomain, err)
 	}

pkg/internal/cyberark/servicediscovery/discovery_test.go

@@ -3,6 +3,11 @@ package servicediscovery
 import (
 	"fmt"
 	"testing"
+
+	"k8s.io/klog/v2"
+	"k8s.io/klog/v2/ktesting"
+
+	_ "k8s.io/klog/v2/ktesting/init"
 )
 
 func Test_DiscoverIdentityAPIURL(t *testing.T) {
@@ -45,12 +50,12 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) {
 
 	for name, testSpec := range tests {
 		t.Run(name, func(t *testing.T) {
-			ctx := t.Context()
+			logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+			ctx := klog.NewContext(t.Context(), logger)
 
-			ts := MockDiscoveryServer()
-			defer ts.Close()
+			httpClient := MockDiscoveryServer(t, mockIdentityAPIURL)
 
-			client := New(WithCustomEndpoint(ts.Server.URL))
+			client := New(httpClient)
 
 			apiURL, err := client.DiscoverIdentityAPIURL(ctx, testSpec.subdomain)
 			if err != nil {

pkg/internal/cyberark/servicediscovery/mock.go

@@ -8,8 +8,11 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"testing"
 	"text/template"
 
+	"k8s.io/client-go/transport"
+
 	"github.com/jetstack/preflight/pkg/version"
 
 	_ "embed"
@@ -19,50 +22,51 @@ const (
 	// MockDiscoverySubdomain is the subdomain for which the MockDiscoveryServer will return a success response
 	MockDiscoverySubdomain = "venafi-test"
 
-	defaultIdentityAPIURL = "https://ajp5871.id.integration-cyberark.cloud"
+	mockIdentityAPIURL = "https://ajp5871.id.integration-cyberark.cloud"
 )
 
 //go:embed testdata/discovery_success.json.template
 var discoverySuccessTemplate string
 
 type mockDiscoveryServer struct {
-	Server *httptest.Server
-
+	t               testing.TB
 	successResponse string
 }
 
-// MockDiscoveryServer returns a mocked discovery server with a default value for the Identity API.
-// The returned server should be Closed by the caller after use.
-func MockDiscoveryServer() *mockDiscoveryServer {
-	return MockDiscoveryServerWithCustomAPIURL(defaultIdentityAPIURL)
-}
-
-func MockDiscoveryServerWithCustomAPIURL(apiURL string) *mockDiscoveryServer {
+// MockDiscoveryServer starts a mocked CyberArk service discovery server and
+// returns an HTTP client with the CA certs needed to connect to it.
+//
+// The URL of the mock server is set in the `ARK_DISCOVERY_API` environment
+// variable, so any code using the `servicediscovery.Client` will use this mock
+// server.
+//
+// The mock server will return a successful response when the subdomain is
+// `MockDiscoverySubdomain`, and the identity API URL in that response will be
+// `identityAPIURL`.
+// Other subdomains, can be used to trigger various failure responses.
+// The returned HTTP client has a transport which logs requests and responses
+// depending on log level of the logger supplied in the context.
+func MockDiscoveryServer(t testing.TB, identityAPIURL string) *http.Client {
 	tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate))
-
 	buf := &bytes.Buffer{}
-
-	err := tmpl.Execute(buf, struct{ IdentityAPIURL string }{apiURL})
+	err := tmpl.Execute(buf, struct{ IdentityAPIURL string }{identityAPIURL})
 	if err != nil {
 		panic(err)
 	}
-
 	mds := &mockDiscoveryServer{
+		t:               t,
 		successResponse: buf.String(),
 	}
-
-	server := httptest.NewServer(mds)
-
-	mds.Server = server
-
-	return mds
-}
-
-func (mds *mockDiscoveryServer) Close() {
-	mds.Server.Close()
+	server := httptest.NewTLSServer(mds)
+	t.Cleanup(server.Close)
+	t.Setenv("ARK_DISCOVERY_API", server.URL)
+	httpClient := server.Client()
+	httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext)
+	return httpClient
 }
 
 func (mds *mockDiscoveryServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	mds.t.Log(r.Method, r.RequestURI)
 	if r.Method != http.MethodGet {
 		// This was observed by making a POST request to the integration environment
 		// Normally, we'd expect 405 Method Not Allowed but we match the observed response here