feat(control-plane): cache GitHub auth tokens and app credentials

[dblinkhorn]

This PR introduces a singleton cache for Octokit clients, authentication objects, and GitHub App configuration, so we can avoid making redundant SSM lookups and client creation.

This PR represents the first step in what will be a broader effort to modularize GitHub interactions. Future PRs will build on this one by adding runner management, job operations, advanced caching for runner and job metadata, etc.

[npalm]

@dblinkhorn thx, not digged in the PR it self. But please can you run yarn format to fix the prettier errors in CI.

[Copilot]

Pull request overview

This PR introduces a singleton caching layer for GitHub API clients, authentication objects, and app configuration to optimize performance by avoiding redundant SSM parameter lookups and client instantiations. The caching implementation uses in-memory Maps for storing Octokit clients, app/installation authentication objects, and GitHub App configuration.

Key changes:

• Created a new cache module (github/cache.ts) with functions to cache and retrieve Octokit clients, authentication objects, and app configs
• Refactored existing authentication functions in auth.ts to utilize the new caching layer
• Added cache reset functionality in test setup to ensure test isolation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
lambdas/functions/control-plane/src/github/types.ts	Defines GithubAppConfig interface for GitHub App configuration with appId, privateKey, and optional installationId
lambdas/functions/control-plane/src/github/index.ts	Exports cache and types modules to make them available to other parts of the application
lambdas/functions/control-plane/src/github/cache.ts	Implements singleton cache using Map structures with cache key generators and getter functions for clients, auth objects, and configs
lambdas/functions/control-plane/src/github/auth.ts	Integrates caching into existing authentication and client creation functions to avoid redundant SSM lookups
lambdas/functions/control-plane/src/github/auth.test.ts	Adds cache reset call in test setup and updates import statement to use type imports
lambdas/functions/control-plane/src/local.ts	Removes an eslint-disable comment that is no longer needed

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Storing tokens directly in cache keys is a security concern as it could expose sensitive credentials in memory dumps or logs. The full token value should not be part of the cache key.

Consider using a hash of the token instead:

import { createHash } from 'crypto';

export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
  const tokenHash = createHash('sha256').update(token).digest('hex').substring(0, 16);
  return `octokit-${tokenHash}-${ghesApiUrl}`;
}

[Copilot]

The installationId field is defined in the GithubAppConfig interface but is never populated or used. It should either be:

1. Removed from the interface if not needed, or
2. Populated in the createAuth function (line 109-112) if it's intended to be part of the config

Currently, the installationId is handled separately in createAuth (line 119) rather than being part of the cached config.

Suggested change

installationId?: number;

[Copilot]

The cache functions and key generation functions lack JSDoc documentation. Adding documentation would help other developers understand:

• The caching strategy and behavior
• When cached values are returned vs when the creator function is called
• The cache key format and what parameters affect it
• The purpose of the reset() function

Example:

/**
 * Creates a cache key for Octokit clients based on token and API URL.
 * @param token - The authentication token
 * @param ghesApiUrl - Optional GitHub Enterprise Server API URL
 * @returns A unique cache key for this token/URL combination
 */
export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
  return `octokit-${token}-${ghesApiUrl}`;
}

[Copilot]

The cache key generation uses installationId || 'none' which treats falsy values (0, null, undefined) the same way. While 0 is likely not a valid GitHub installation ID, this could lead to cache key collisions for different falsy values.

Consider using nullish coalescing instead for more precise handling:

const id = installationId ?? 'none';

This will only treat null and undefined as 'none', while preserving 0 if it were ever a valid value.

Suggested change

	const id = installationId || 'none';
	const id = installationId ?? 'none';

[npalm]

seems valid

[dblinkhorn]

Fixed.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

lambdas/functions/control-plane/src/github/auth.test.ts:38

• Consider adding test cases to verify that the caching behavior works correctly. For example:

• Verify that calling createGithubInstallationAuth twice with the same parameters only calls the underlying createAuth once (due to caching)
• Verify that calling createGithubInstallationAuth with different installationId or ghesApiUrl values creates separate cache entries
• Verify that the reset() function properly clears the cache so subsequent calls fetch fresh values

This would ensure the caching logic introduced in this PR works as intended.

beforeEach(() => {
  reset(); // clear all caches before each test
  vi.resetModules();
  vi.clearAllMocks();
  process.env = { ...cleanEnv };
  process.env.PARAMETER_GITHUB_APP_ID_NAME = PARAMETER_GITHUB_APP_ID_NAME;
  process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME = PARAMETER_GITHUB_APP_KEY_BASE64_NAME;
  nock.disableNetConnect();
});

lambdas/functions/control-plane/src/github/auth.ts:91

• String argument /[\\n]/g looks like a regular expression, but it will be interpreted as a string.

    const privateKey = Buffer.from(
      await getParameter(process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME),
      'base64',
      // replace literal \n characters with new lines to allow the key to be stored as a
      // single line variable. This logic should match how the GitHub Terraform provider
      // processes private keys to retain compatibility between the projects
    )
      .toString()
      .replace('/[\\n]/g', String.fromCharCode(10));

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The regex pattern is incorrectly written as a string literal '/[\\n]/g' instead of an actual regex /\\n/g. This means the replace operation will look for the literal string /[\\n]/g instead of matching newline characters. The code should use .replace(/\\n/g, String.fromCharCode(10)) to properly replace literal \n sequences with actual newline characters.

Note: While this bug existed before this PR, it's being surfaced here because the line appears in the diff (due to the comma → semicolon change).

Suggested change

	.replace('/[\\n]/g', String.fromCharCode(10));
	.replace(/\\n/g, String.fromCharCode(10));

[npalm]

@dblinkhorn although this seems to be a valid comment. I tihink it is better to leave and create a seperate issue.

Alternative is indeed to fix it, but in that case I prefer to have a unit test.

Since potential problem is not introduced here. Leave it up to you.

[dblinkhorn]

I just went ahead and fixed it and added a new test.

[dblinkhorn]

@npalm lemme know which of these copilot suggestions you'd like me to implement.

[npalm]

@npalm lemme know which of these copilot suggestions you'd like me to implement.

I hope to find later the week a bit of time to go through. Feel free to judge yourself and leave some comments. Copilot is not alwasy very useful. Just giving it a shot.

[npalm]

@dblinkhorn I have changed the target branch. Since we are busy to get a a new release out. I just want to see if there are any conflicts.

[guicaulada]

I have confirmed this works, however I'm testing further to verify the impact of the cache on the amount of requests.

The first thing that comes to mind, is that this implementation is very specific for GitHub Authentication while we could implement a more generic cache library that could be used by all lambdas in different areas with ease.

@npalm @dblinkhorn what do you think about converting this to a more generic cache implementation that can be used in more places in the future?