Add Identifier to actions.Client (#2237)

github/actions/client.go

@@ -165,6 +165,29 @@ func NewClient(githubConfigURL string, creds *ActionsAuth, options ...ClientOpti
 	return ac, nil
 }
 
+// Identifier returns a string to help identify a client uniquely.
+// This is used for caching client instances and understanding when a config
+// change warrants creating a new client. Any changes to Client that would
+// require a new client should be reflected here.
+func (c *Client) Identifier() string {
+	identifier := fmt.Sprintf("configURL:%q,", c.config.ConfigURL.String())
+
+	if c.creds.Token != "" {
+		identifier += fmt.Sprintf("token:%q", c.creds.Token)
+	}
+
+	if c.creds.AppCreds != nil {
+		identifier += fmt.Sprintf(
+			"appID:%q,installationID:%q,key:%q",
+			c.creds.AppCreds.AppID,
+			c.creds.AppCreds.AppInstallationID,
+			c.creds.AppCreds.AppPrivateKey,
+		)
+	}
+
+	return uuid.NewMD5(uuid.NameSpaceOID, []byte(identifier)).String()
+}
+
 func (c *Client) Do(req *http.Request) (*http.Response, error) {
 	resp, err := c.Client.Do(req)
 	if err != nil {

github/actions/config_test.go

@@ -91,7 +91,7 @@ func TestGitHubConfig(t *testing.T) {
 		}
 	})
 
-	t.Run("when given an invalid URL", func(t *testing.T) {})
+	t.Run("when given an invalid URL", func(t *testing.T) {
 		invalidURLs := []string{
 			"https://github.com/",
 			"https://github.com",
@@ -103,6 +103,7 @@ func TestGitHubConfig(t *testing.T) {
 			require.Error(t, err)
 			assert.True(t, errors.Is(err, actions.ErrInvalidGitHubConfigURL))
 		}
+	})
 }
 
 func TestGitHubConfig_GitHubAPIURL(t *testing.T) {

github/actions/identifier_test.go

@@ -0,0 +1,111 @@
+package actions_test
+
+import (
+	"testing"
+
+	"github.com/actions/actions-runner-controller/github/actions"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestClient_Identifier(t *testing.T) {
+	t.Run("configURL changes", func(t *testing.T) {
+		scenarios := []struct {
+			name string
+			url  string
+		}{
+			{
+				name: "url of a different repo",
+				url:  "https://github.com/org/repo2",
+			},
+			{
+				name: "url of an org",
+				url:  "https://github.com/org",
+			},
+			{
+				name: "url of an enterprise",
+				url:  "https://github.com/enterprises/my-enterprise",
+			},
+			{
+				name: "url of a self-hosted github",
+				url:  "https://selfhosted.com/org/repo",
+			},
+		}
+
+		configURL := "https://github.com/org/repo"
+		defaultCreds := &actions.ActionsAuth{
+			Token: "token",
+		}
+		oldClient, err := actions.NewClient(configURL, defaultCreds)
+		require.NoError(t, err)
+
+		for _, scenario := range scenarios {
+			t.Run(scenario.name, func(t *testing.T) {
+				newClient, err := actions.NewClient(scenario.url, defaultCreds)
+				require.NoError(t, err)
+				assert.NotEqual(t, oldClient.Identifier(), newClient.Identifier())
+			})
+		}
+	})
+
+	t.Run("credentials change", func(t *testing.T) {
+		defaultTokenCreds := &actions.ActionsAuth{
+			Token: "token",
+		}
+		defaultAppCreds := &actions.ActionsAuth{
+			AppCreds: &actions.GitHubAppAuth{
+				AppID:             123,
+				AppInstallationID: 123,
+				AppPrivateKey:     "private key",
+			},
+		}
+
+		scenarios := []struct {
+			name string
+			old  *actions.ActionsAuth
+			new  *actions.ActionsAuth
+		}{
+			{
+				name: "different token",
+				old:  defaultTokenCreds,
+				new: &actions.ActionsAuth{
+					Token: "new token",
+				},
+			},
+			{
+				name: "changing from token to github app",
+				old:  defaultTokenCreds,
+				new:  defaultAppCreds,
+			},
+			{
+				name: "changing from github app to token",
+				old:  defaultAppCreds,
+				new:  defaultTokenCreds,
+			},
+			{
+				name: "different github app",
+				old:  defaultAppCreds,
+				new: &actions.ActionsAuth{
+					AppCreds: &actions.GitHubAppAuth{
+						AppID:             456,
+						AppInstallationID: 456,
+						AppPrivateKey:     "new private key",
+					},
+				},
+			},
+		}
+
+		defaultConfigURL := "https://github.com/org/repo"
+
+		for _, scenario := range scenarios {
+			t.Run(scenario.name, func(t *testing.T) {
+				oldClient, err := actions.NewClient(defaultConfigURL, scenario.old)
+				require.NoError(t, err)
+
+				newClient, err := actions.NewClient(defaultConfigURL, scenario.new)
+				require.NoError(t, err)
+				assert.NotEqual(t, oldClient.Identifier(), newClient.Identifier())
+			})
+		}
+	})
+}

github/actions/multi_client.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
-	"net/url"
 	"strconv"
 	"sync"
 
@@ -19,7 +18,7 @@ type MultiClient interface {
 type multiClient struct {
 	// To lock adding and removing of individual clients.
 	mu      sync.Mutex
-	clients map[ActionsClientKey]*actionsClientWrapper
+	clients map[ActionsClientKey]*Client
 
 	logger    logr.Logger
 	userAgent string
@@ -40,22 +39,14 @@ type ActionsAuth struct {
 }
 
 type ActionsClientKey struct {
-	ActionsURL string
+	Identifier string
-	Auth       ActionsAuth
 	Namespace  string
 }
 
-type actionsClientWrapper struct {
-	// To lock client usage when tokens are being refreshed.
-	mu sync.Mutex
-
-	client ActionsService
-}
-
 func NewMultiClient(userAgent string, logger logr.Logger) MultiClient {
 	return &multiClient{
 		mu:        sync.Mutex{},
-		clients:   make(map[ActionsClientKey]*actionsClientWrapper),
+		clients:   make(map[ActionsClientKey]*Client),
 		logger:    logger,
 		userAgent: userAgent,
 	}
@@ -64,11 +55,6 @@ func NewMultiClient(userAgent string, logger logr.Logger) MultiClient {
 func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string, creds ActionsAuth, namespace string) (ActionsService, error) {
 	m.logger.Info("retrieve actions client", "githubConfigURL", githubConfigURL, "namespace", namespace)
 
-	parsedGitHubURL, err := url.Parse(githubConfigURL)
-	if err != nil {
-		return nil, err
-	}
-
 	if creds.Token == "" && creds.AppCreds == nil {
 		return nil, fmt.Errorf("no credentials provided. either a PAT or GitHub App credentials should be provided")
 	}
@@ -77,34 +63,6 @@ func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string,
 		return nil, fmt.Errorf("both PAT and GitHub App credentials provided. should only provide one")
 	}
 
-	key := ActionsClientKey{
-		ActionsURL: parsedGitHubURL.String(),
-		Namespace:  namespace,
-	}
-
-	if creds.AppCreds != nil {
-		key.Auth = ActionsAuth{
-			AppCreds: creds.AppCreds,
-		}
-	}
-
-	if creds.Token != "" {
-		key.Auth = ActionsAuth{
-			Token: creds.Token,
-		}
-	}
-
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	clientWrapper, has := m.clients[key]
-	if has {
-		m.logger.Info("using cache client", "githubConfigURL", githubConfigURL, "namespace", namespace)
-		return clientWrapper.client, nil
-	}
-
-	m.logger.Info("creating new client", "githubConfigURL", githubConfigURL, "namespace", namespace)
-
 	client, err := NewClient(
 		githubConfigURL,
 		&creds,
@@ -115,11 +73,24 @@ func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string,
 		return nil, err
 	}
 
-	m.clients[key] = &actionsClientWrapper{
+	m.mu.Lock()
-		mu:     sync.Mutex{},
+	defer m.mu.Unlock()
-		client: client,
+
+	key := ActionsClientKey{
+		Identifier: client.Identifier(),
+		Namespace:  namespace,
 	}
 
+	cachedClient, has := m.clients[key]
+	if has {
+		m.logger.Info("using cache client", "githubConfigURL", githubConfigURL, "namespace", namespace)
+		return cachedClient, nil
+	}
+
+	m.logger.Info("creating new client", "githubConfigURL", githubConfigURL, "namespace", namespace)
+
+	m.clients[key] = client
+
 	m.logger.Info("successfully created new client", "githubConfigURL", githubConfigURL, "namespace", namespace)
 
 	return client, nil

github/actions/multi_client_test.go

@@ -2,131 +2,51 @@ package actions
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"net/http"
-	"net/http/httptest"
-	"strings"
 	"testing"
-	"time"
 
 	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func TestAddClient(t *testing.T) {
+func TestMultiClientCaching(t *testing.T) {
 	logger := logr.Discard()
+	ctx := context.Background()
 	multiClient := NewMultiClient("test-user-agent", logger).(*multiClient)
 
-	ctx := context.Background()
+	defaultNamespace := "default"
+	defaultConfigURL := "https://github.com/org/repo"
+	defaultCreds := &ActionsAuth{
+		Token: "token",
+	}
+	client, err := NewClient(defaultConfigURL, defaultCreds)
+	require.NoError(t, err)
 
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	multiClient.clients[ActionsClientKey{client.Identifier(), defaultNamespace}] = client
-		if strings.HasSuffix(r.URL.Path, "actions/runners/registration-token") {
-			w.WriteHeader(http.StatusCreated)
-			w.Header().Set("Content-Type", "application/json")
 
-			token := "abc-123"
+	// Verify that the client is cached
-			rt := &registrationToken{Token: &token}
+	cachedClient, err := multiClient.GetClientFor(
+		ctx,
+		defaultConfigURL,
+		*defaultCreds,
+		defaultNamespace,
+	)
+	require.NoError(t, err)
+	assert.Equal(t, client, cachedClient)
+	assert.Len(t, multiClient.clients, 1)
 
-			if err := json.NewEncoder(w).Encode(rt); err != nil {
+	// Asking for a different client results in creating and caching a new client
-				http.Error(w, err.Error(), http.StatusInternalServerError)
+	otherNamespace := "other"
-				return
+	newClient, err := multiClient.GetClientFor(
-			}
+		ctx,
-		}
+		defaultConfigURL,
-		if strings.HasSuffix(r.URL.Path, "actions/runner-registration") {
+		*defaultCreds,
-			w.Header().Set("Content-Type", "application/json")
+		otherNamespace,
-
+	)
-			url := "actions.github.com/abc"
+	require.NoError(t, err)
-			jwt := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjI1MTYyMzkwMjJ9.tlrHslTmDkoqnc4Kk9ISoKoUNDfHo-kjlH-ByISBqzE"
+	assert.NotEqual(t, client, newClient)
-			adminConnInfo := &ActionsServiceAdminConnection{ActionsServiceUrl: &url, AdminToken: &jwt}
+	assert.Len(t, multiClient.clients, 2)
-
-			if err := json.NewEncoder(w).Encode(adminConnInfo); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		}
-		if strings.HasSuffix(r.URL.Path, "/access_tokens") {
-			w.Header().Set("Content-Type", "application/vnd.github+json")
-
-			t, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z07:00")
-			accessToken := &accessToken{
-				Token:     "abc-123",
-				ExpiresAt: t,
-			}
-
-			if err := json.NewEncoder(w).Encode(accessToken); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		}
-	}))
-	defer srv.Close()
-
-	want := 1
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{Token: "PAT"}, "namespace"); err != nil {
-		t.Fatal(err)
-	}
-
-	want++ // New repo
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/actions", srv.URL), ActionsAuth{Token: "PAT"}, "namespace"); err != nil {
-		t.Fatal(err)
-	}
-
-	// Repeat
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{Token: "PAT"}, "namespace"); err != nil {
-		t.Fatal(err)
-	}
-
-	want++ // New namespace
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{Token: "PAT"}, "other"); err != nil {
-		t.Fatal(err)
-	}
-
-	want++ // New pat
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{Token: "other"}, "other"); err != nil {
-		t.Fatal(err)
-	}
-
-	want++ // New org
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github", srv.URL), ActionsAuth{Token: "PAT"}, "other"); err != nil {
-		t.Fatal(err)
-	}
-
-	// No org, repo, enterprise
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v", srv.URL), ActionsAuth{Token: "PAT"}, "other"); err == nil {
-		t.Fatal(err)
-	}
-
-	want++ // Test keying on GitHub App
-	appAuth := &GitHubAppAuth{
-		AppID: 1,
-		AppPrivateKey: `-----BEGIN RSA PRIVATE KEY-----
-MIICWgIBAAKBgHXfRT9cv9UY9fAAD4+1RshpfSSZe277urfEmPfX3/Og9zJYRk//
-CZrJVD1CaBZDiIyQsNEzjta7r4UsqWdFOggiNN2E7ZTFQjMSaFkVgrzHqWuiaCBf
-/BjbKPn4SMDmTzHvIe7Nel76hBdCaVgu6mYCW5jmuSH5qz/yR1U1J/WJAgMBAAEC
-gYARWGWsSU3BYgbu5lNj5l0gKMXNmPhdAJYdbMTF0/KUu18k/XB7XSBgsre+vALt
-I8r4RGKApoGif8P4aPYUyE8dqA1bh0X3Fj1TCz28qoUL5//dA+pigCRS20H7HM3C
-ojoqF7+F+4F2sXmzFNd1NgY5RxFPYosTT7OnUiFuu2IisQJBALnMLe09LBnjuHXR
-xxR65DDNxWPQLBjW3dL+ubLcwr7922l6ZIQsVjdeE0ItEUVRjjJ9/B/Jq9VJ/Lw4
-g9LCkkMCQQCiaM2f7nYmGivPo9hlAbq5lcGJ5CCYFfeeYzTxMqum7Mbqe4kk5lgb
-X6gWd0Izg2nGdAEe/97DClO6VpKcPbpDAkBTR/JOJN1fvXMxXJaf13XxakrQMr+R
-Yr6LlSInykyAz8lJvlLP7A+5QbHgN9NF/wh+GXqpxPwA3ukqdSqhjhWBAkBn6mDv
-HPgR5xrzL6XM8y9TgaOlJAdK6HtYp6d/UOmN0+Butf6JUq07TphRT5tXNJVgemch
-O5x/9UKfbrc+KyzbAkAo97TfFC+mZhU1N5fFelaRu4ikPxlp642KRUSkOh8GEkNf
-jQ97eJWiWtDcsMUhcZgoB5ydHcFlrBIn6oBcpge5
------END RSA PRIVATE KEY-----`,
-	}
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{AppCreds: appAuth}, "other"); err != nil {
-		t.Fatal(err)
-	}
-
-	// Repeat last to verify GitHub App keys are mapped together
-	if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{AppCreds: appAuth}, "other"); err != nil {
-		t.Fatal(err)
-	}
-
-	if len(multiClient.clients) != want {
-		t.Fatalf("GetClientFor: unexpected number of clients: got=%v want=%v", len(multiClient.clients), want)
-	}
 }
 
 func TestCreateJWT(t *testing.T) {