Merge pull request #3853 from github/mbg/start-proxy/improved-checks

Improve connection tests

src/doc-url.ts

@@ -8,6 +8,7 @@ export enum DocUrl {
   CODEQL_BUILD_MODES = "https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages#codeql-build-modes",
   DEFINE_ENV_VARIABLES = "https://docs.github.com/en/actions/learn-github-actions/variables#defining-environment-variables-for-a-single-workflow",
   DELETE_ACTIONS_CACHE_ENTRIES = "https://docs.github.com/en/actions/how-tos/manage-workflow-runs/manage-caches#deleting-cache-entries",
+  PRIVATE_REGISTRY_LOGS = "https://docs.github.com/en/code-security/reference/code-scanning/code-scanning-logs#diagnostic-information-for-private-package-registries",
   SCANNING_ON_PUSH = "https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#scanning-on-push",
   SPECIFY_BUILD_STEPS_MANUALLY = "https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages#about-specifying-build-steps-manually",
   SYSTEM_REQUIREMENTS = "https://codeql.github.com/docs/codeql-overview/system-requirements/",

src/start-proxy-action.ts

@@ -111,7 +111,7 @@ async function run(startedAt: Date) {
       logger,
     );
 
-    // Check that the private registries are reachable.
+    // Perform best-effort checks that the private registries are reachable.
     await checkConnections(logger, proxyInfo);
 
     // Report success if we have reached this point.

src/start-proxy/reachability.test.ts

@@ -8,6 +8,7 @@ import {
 } from "./../testing-utils";
 import {
   checkConnections,
+  connectionTestConfig,
   ReachabilityBackend,
   ReachabilityError,
 } from "./reachability";
@@ -118,3 +119,34 @@ test("checkConnections - handles invalid URLs", async (t) => {
     `Finished testing connections`,
   ]);
 });
+
+test("checkConnections - appends extra paths", async (t) => {
+  const backend = new MockReachabilityBackend();
+  const checkConnection = sinon.stub(backend, "checkConnection").resolves(200);
+
+  const messages = await withRecordingLoggerAsync(async (logger) => {
+    await checkConnections(
+      logger,
+      {
+        ...proxyInfo,
+        registries: [{ ...nugetFeed, url: "https://api.nuget.org/" }],
+      },
+      backend,
+    );
+  });
+  checkExpectedLogMessages(t, messages, [
+    `Testing connection to https://api.nuget.org/`,
+    `Successfully tested connection to https://api.nuget.org/`,
+    `Finished testing connections`,
+  ]);
+
+  t.true(
+    checkConnection.calledWith(
+      sinon.match(
+        new URL(
+          `https://api.nuget.org/${connectionTestConfig["nuget_feed"]?.path}`,
+        ),
+      ),
+    ),
+  );
+});

src/start-proxy/reachability.ts

@@ -2,11 +2,41 @@ import * as https from "https";
 
 import { HttpsProxyAgent } from "https-proxy-agent";
 
+import { DocUrl } from "../doc-url";
 import { Logger } from "../logging";
 import { getErrorMessage } from "../util";
 
 import { getAddressString, ProxyInfo, Registry } from "./types";
 
+/** Represents registry-specific connection test configurations. */
+export interface ConnectionTestConfig {
+  /** An optional path to append to the end of the base url. */
+  path?: string;
+}
+
+/** A partial mapping of registry types to extra connection test configurations. */
+export const connectionTestConfig: Partial<
+  Record<string, ConnectionTestConfig>
+> = {
+  nuget_feed: { path: "v3/index.json" },
+};
+
+/**
+ * Applies the registry-specific check configuration to the base URL, if any and applicable.
+ */
+export function makeTestUrl(
+  config: ConnectionTestConfig | undefined,
+  base: URL,
+): URL {
+  if (config?.path === undefined) {
+    return base;
+  }
+  if (base.pathname.endsWith(config.path)) {
+    return base;
+  }
+  return new URL(config.path, base);
+}
+
 export class ReachabilityError extends Error {
   constructor(public readonly statusCode?: number | undefined) {
     super();
@@ -41,7 +71,7 @@ class NetworkReachabilityBackend implements ReachabilityBackend {
         url,
         {
           agent: this.agent,
-          method: "HEAD",
+          method: "GET",
           ca: this.proxy.cert,
           timeout: 5 * 1000, // 5 seconds
         },
@@ -85,6 +115,13 @@ export async function checkConnections(
   // Don't do anything if there are no registries.
   if (proxy.registries.length === 0) return result;
 
+  // Start a log group and print a message with a disclaimer with a link to the
+  // relevant documentation that these checks are a best-effort process.
+  logger.startGroup("Testing connections via the proxy");
+  logger.info(
+    `The connection tests performed here are best-effort only and failures here may not affect the subsequent analysis. See ${DocUrl.PRIVATE_REGISTRY_LOGS} for more information.`,
+  );
+
   try {
     // Initialise a networking backend if no backend was provided.
     if (backend === undefined) {
@@ -92,6 +129,7 @@ export async function checkConnections(
     }
 
     for (const registry of proxy.registries) {
+      const config = connectionTestConfig[registry.type];
       const address = getAddressString(registry);
       const url = URL.parse(address);
 
@@ -102,9 +140,11 @@ export async function checkConnections(
         continue;
       }
 
+      const testUrl = makeTestUrl(config, url);
+
       try {
         logger.debug(`Testing connection to ${url}...`);
-        const statusCode = await backend.checkConnection(url);
+        const statusCode = await backend.checkConnection(testUrl);
 
         logger.info(`Successfully tested connection to ${url} (${statusCode})`);
         result.add(registry);
@@ -126,5 +166,6 @@ export async function checkConnections(
     );
   }
 
+  logger.endGroup();
   return result;
 }