diff --git a/github/actions/client.go b/github/actions/client.go index c447f7cf..63c9ed16 100644 --- a/github/actions/client.go +++ b/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 { diff --git a/github/actions/config_test.go b/github/actions/config_test.go index a9a8368f..e64928e2 100644 --- a/github/actions/config_test.go +++ b/github/actions/config_test.go @@ -91,18 +91,19 @@ func TestGitHubConfig(t *testing.T) { } }) - t.Run("when given an invalid URL", func(t *testing.T) {}) - invalidURLs := []string{ - "https://github.com/", - "https://github.com", - "https://github.com/some/random/path", - } + t.Run("when given an invalid URL", func(t *testing.T) { + invalidURLs := []string{ + "https://github.com/", + "https://github.com", + "https://github.com/some/random/path", + } - for _, u := range invalidURLs { - _, err := actions.ParseGitHubConfigFromURL(u) - require.Error(t, err) - assert.True(t, errors.Is(err, actions.ErrInvalidGitHubConfigURL)) - } + for _, u := range invalidURLs { + _, err := actions.ParseGitHubConfigFromURL(u) + require.Error(t, err) + assert.True(t, errors.Is(err, actions.ErrInvalidGitHubConfigURL)) + } + }) } func TestGitHubConfig_GitHubAPIURL(t *testing.T) { diff --git a/github/actions/identifier_test.go b/github/actions/identifier_test.go new file mode 100644 index 00000000..0a184f86 --- /dev/null +++ b/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()) + }) + } + }) +} diff --git a/github/actions/multi_client.go b/github/actions/multi_client.go index b875c872..1e204139 100644 --- a/github/actions/multi_client.go +++ b/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 - Auth ActionsAuth + Identifier string 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{ - mu: sync.Mutex{}, - client: client, + m.mu.Lock() + defer m.mu.Unlock() + + 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 diff --git a/github/actions/multi_client_test.go b/github/actions/multi_client_test.go index fb4a64db..a61686c6 100644 --- a/github/actions/multi_client_test.go +++ b/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() - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.HasSuffix(r.URL.Path, "actions/runners/registration-token") { - w.WriteHeader(http.StatusCreated) - w.Header().Set("Content-Type", "application/json") - - token := "abc-123" - rt := ®istrationToken{Token: &token} - - if err := json.NewEncoder(w).Encode(rt); err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - } - if strings.HasSuffix(r.URL.Path, "actions/runner-registration") { - w.Header().Set("Content-Type", "application/json") - - url := "actions.github.com/abc" - jwt := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjI1MTYyMzkwMjJ9.tlrHslTmDkoqnc4Kk9ISoKoUNDfHo-kjlH-ByISBqzE" - adminConnInfo := &ActionsServiceAdminConnection{ActionsServiceUrl: &url, AdminToken: &jwt} - - 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) + defaultNamespace := "default" + defaultConfigURL := "https://github.com/org/repo" + defaultCreds := &ActionsAuth{ + Token: "token", } + client, err := NewClient(defaultConfigURL, defaultCreds) + require.NoError(t, 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) - } + multiClient.clients[ActionsClientKey{client.Identifier(), defaultNamespace}] = client - // Repeat - if _, err := multiClient.GetClientFor(ctx, fmt.Sprintf("%v/github/github", srv.URL), ActionsAuth{Token: "PAT"}, "namespace"); err != nil { - t.Fatal(err) - } + // Verify that the client is cached + cachedClient, err := multiClient.GetClientFor( + ctx, + defaultConfigURL, + *defaultCreds, + defaultNamespace, + ) + require.NoError(t, err) + assert.Equal(t, client, cachedClient) + assert.Len(t, multiClient.clients, 1) - 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) - } + // Asking for a different client results in creating and caching a new client + otherNamespace := "other" + newClient, err := multiClient.GetClientFor( + ctx, + defaultConfigURL, + *defaultCreds, + otherNamespace, + ) + require.NoError(t, err) + assert.NotEqual(t, client, newClient) + assert.Len(t, multiClient.clients, 2) } func TestCreateJWT(t *testing.T) {