fix(cli): calling cli with --no-notices makes an unexpected network call to refresh notices

Author: mrgrain

.projenrc.ts

@@ -1010,6 +1010,7 @@ const cli = configureProject(
       'jest-environment-node',
       'jest-mock',
       'madge',
+      'nock@13',
       'sinon',
       'ts-mock-imports',
       'xml-js',

packages/@aws-cdk/toolkit-lib/lib/api/notices/cached-data-source.ts

@@ -1,6 +1,7 @@
 import * as fs from 'fs-extra';
 import type { Notice, NoticeDataSource } from './types';
 import type { IoDefaultMessages } from '../io/private';
+import { ToolkitError } from '../toolkit-error';
 
 interface CachedNotices {
   expiration: number;
@@ -15,7 +16,8 @@ export class CachedDataSource implements NoticeDataSource {
     private readonly ioMessages: IoDefaultMessages,
     private readonly fileName: string,
     private readonly dataSource: NoticeDataSource,
-    private readonly skipCache?: boolean) {
+    private readonly skipCache?: boolean,
+  ) {
   }
 
   async fetch(): Promise<Notice[]> {
@@ -24,28 +26,32 @@ export class CachedDataSource implements NoticeDataSource {
     const expiration = cachedData.expiration ?? 0;
 
     if (Date.now() > expiration || this.skipCache) {
-      const freshData = await this.fetchInner();
-      await this.save(freshData);
-      return freshData.notices;
+      let updatedData: CachedNotices = cachedData;
+
+      try {
+        updatedData = await this.fetchInner();
+      } catch (e) {
+        this.ioMessages.debug(`Could not refresh notices: ${e}`);
+        updatedData = {
+          expiration: Date.now() + TIME_TO_LIVE_ERROR,
+          notices: [],
+        };
+        throw ToolkitError.withCause('Failed to load CDK notices. Please try again later.', e);
+      } finally {
+        await this.save(updatedData);
+      }
+      return updatedData.notices;
     } else {
       this.ioMessages.debug(`Reading cached notices from ${this.fileName}`);
       return data;
     }
   }
 
   private async fetchInner(): Promise<CachedNotices> {
-    try {
-      return {
-        expiration: Date.now() + TIME_TO_LIVE_SUCCESS,
-        notices: await this.dataSource.fetch(),
-      };
-    } catch (e) {
-      this.ioMessages.debug(`Could not refresh notices: ${e}`);
-      return {
-        expiration: Date.now() + TIME_TO_LIVE_ERROR,
-        notices: [],
-      };
-    }
+    return {
+      expiration: Date.now() + TIME_TO_LIVE_SUCCESS,
+      notices: await this.dataSource.fetch(),
+    };
   }
 
   private async load(): Promise<CachedNotices> {

packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts

@@ -4,6 +4,7 @@ import type { SdkHttpOptions } from '../aws-auth';
 import type { Context } from '../context';
 import type { IIoHost } from '../io';
 import { CachedDataSource } from './cached-data-source';
+import type { FilteredNotice } from './filter';
 import { NoticesFilter } from './filter';
 import type { BootstrappedEnvironment, Notice, NoticeDataSource } from './types';
 import { WebsiteNoticeDataSource } from './web-data-source';
@@ -18,13 +19,6 @@ export interface NoticesProps {
    */
   readonly context: Context;
 
-  /**
-   * Include notices that have already been acknowledged.
-   *
-   * @default false
-   */
-  readonly includeAcknowledged?: boolean;
-
   /**
    * Global CLI option for output directory for synthesized cloud assembly
    *
@@ -48,8 +42,16 @@ export interface NoticesProps {
   readonly cliVersion: string;
 }
 
-export interface NoticesPrintOptions {
+export interface NoticesFilterOptions {
+  /**
+   * Include notices that have already been acknowledged.
+   *
+   * @default false
+   */
+  readonly includeAcknowledged?: boolean;
+}
 
+export interface NoticesDisplayOptions extends NoticesFilterOptions {
   /**
    * Whether to append the total number of unacknowledged notices to the display.
    *
@@ -98,7 +100,6 @@ export class Notices {
   private readonly context: Context;
   private readonly output: string;
   private readonly acknowledgedIssueNumbers: Set<Number>;
-  private readonly includeAcknowlegded: boolean;
   private readonly httpOptions: SdkHttpOptions;
   private readonly ioHelper: IoHelper;
   private readonly ioMessages: IoDefaultMessages;
@@ -112,7 +113,6 @@ export class Notices {
   private constructor(props: NoticesProps) {
     this.context = props.context;
     this.acknowledgedIssueNumbers = new Set(this.context.get('acknowledged-issue-numbers') ?? []);
-    this.includeAcknowlegded = props.includeAcknowledged ?? false;
     this.output = props.output ?? 'cdk.out';
     this.httpOptions = props.httpOptions ?? {};
     this.ioHelper = asIoHelper(props.ioHost, 'notices' as any /* forcing a CliAction to a ToolkitAction */);
@@ -136,35 +136,39 @@ export class Notices {
 
   /**
    * Refresh the list of notices this instance is aware of.
-   * To make sure this never crashes the CLI process, all failures are caught and
-   * silently logged.
    *
-   * If context is configured to not display notices, this will no-op.
+   * This method throws an error if the data source fails to fetch notices.
+   * When using, consider if execution should halt immdiately or if catching the rror and continuing is more appropriate
+   *
+   * @throws on failure to refresh the data source
    */
   public async refresh(options: NoticesRefreshOptions = {}) {
-    try {
-      const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.ioHelper, this.httpOptions);
-      const dataSource = new CachedDataSource(this.ioMessages, CACHE_FILE_PATH, underlyingDataSource, options.force ?? false);
-      const notices = await dataSource.fetch();
-      this.data = new Set(this.includeAcknowlegded ? notices : notices.filter(n => !this.acknowledgedIssueNumbers.has(n.issueNumber)));
-    } catch (e: any) {
-      this.ioMessages.debug(`Could not refresh notices: ${e}`);
-    }
+    const innerDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.ioHelper, this.httpOptions);
+    const dataSource = new CachedDataSource(this.ioMessages, CACHE_FILE_PATH, innerDataSource, options.force ?? false);
+    const notices = await dataSource.fetch();
+    this.data = new Set(notices);
   }
 
   /**
-   * Display the relevant notices (unless context dictates we shouldn't).
+   * Filter the data sourece for relevant notices
    */
-  public display(options: NoticesPrintOptions = {}) {
-    const filteredNotices = new NoticesFilter(this.ioMessages).filter({
-      data: Array.from(this.data),
+  public filter(options: NoticesDisplayOptions = {}): FilteredNotice[] {
+    return new NoticesFilter(this.ioMessages).filter({
+      data: this.noticesFromData(options.includeAcknowledged ?? false),
       cliVersion: this.cliVersion,
       outDir: this.output,
       bootstrappedEnvironments: Array.from(this.bootstrappedEnvironments.values()),
     });
+  }
+
+  /**
+   * Display the relevant notices (unless context dictates we shouldn't).
+   */
+  public async display(options: NoticesDisplayOptions = {}): Promise<void> {
+    const filteredNotices = this.filter(options);
 
     if (filteredNotices.length > 0) {
-      void this.ioMessages.notify(IO.CDK_TOOLKIT_I0100.msg([
+      await this.ioHelper.notify(IO.CDK_TOOLKIT_I0100.msg([
         '',
         'NOTICES         (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)',
         '',
@@ -173,25 +177,41 @@ export class Notices {
         const formatted = filtered.format() + '\n';
         switch (filtered.notice.severity) {
           case 'warning':
-            void this.ioMessages.notify(IO.CDK_TOOLKIT_W0101.msg(formatted));
+            await this.ioHelper.notify(IO.CDK_TOOLKIT_W0101.msg(formatted));
             break;
           case 'error':
-            void this.ioMessages.notify(IO.CDK_TOOLKIT_E0101.msg(formatted));
+            await this.ioHelper.notify(IO.CDK_TOOLKIT_E0101.msg(formatted));
             break;
           default:
-            void this.ioMessages.notify(IO.CDK_TOOLKIT_I0101.msg(formatted));
+            await this.ioHelper.notify(IO.CDK_TOOLKIT_I0101.msg(formatted));
             break;
         }
       }
-      void this.ioMessages.notify(IO.CDK_TOOLKIT_I0100.msg(
+      await this.ioHelper.notify(IO.CDK_TOOLKIT_I0100.msg(
         `If you don’t want to see a notice anymore, use "cdk acknowledge <id>". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".`,
       ));
     }
 
     if (options.showTotal ?? false) {
-      void this.ioMessages.notify(IO.CDK_TOOLKIT_I0100.msg(
+      await this.ioHelper.notify(IO.CDK_TOOLKIT_I0100.msg(
         `\nThere are ${filteredNotices.length} unacknowledged notice(s).`,
       ));
     }
   }
+
+  /**
+   * List all notices available in the data source.
+   *
+   * @param includeAcknowlegded Whether to include acknowledged notices.
+   */
+  private noticesFromData(includeAcknowlegded = false): Notice[] {
+    const data = Array.from(this.data);
+
+    if (includeAcknowlegded) {
+      return data;
+    }
+
+    return data.filter(n => !this.acknowledgedIssueNumbers.has(n.issueNumber));
+  }
 }
+

packages/@aws-cdk/toolkit-lib/lib/api/notices/web-data-source.ts

@@ -1,13 +1,13 @@
 import type { ClientRequest } from 'http';
 import type { RequestOptions } from 'https';
 import * as https from 'node:https';
-import { formatErrorMessage } from '../../util';
 import type { SdkHttpOptions } from '../aws-auth';
 import { ProxyAgentProvider } from '../aws-auth/private';
 import type { IoHelper } from '../io/private';
 import { IO } from '../io/private';
 import { ToolkitError } from '../toolkit-error';
 import type { Notice, NoticeDataSource } from './types';
+import { formatErrorMessage, humanHttpStatusError, humanNetworkError } from '../../util';
 
 export class WebsiteNoticeDataSource implements NoticeDataSource {
   private readonly options: SdkHttpOptions;
@@ -48,23 +48,25 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {
                 try {
                   const data = JSON.parse(rawData).notices as Notice[];
                   if (!data) {
-                    throw new ToolkitError("'notices' key is missing");
+                    throw new ToolkitError("'notices' key is missing from received data");
                   }
                   resolve(data ?? []);
                 } catch (e: any) {
-                  reject(new ToolkitError(`Failed to parse notices: ${formatErrorMessage(e)}`));
+                  reject(ToolkitError.withCause(`Parse error: ${formatErrorMessage(e)}`, e));
                 }
               });
               res.on('error', e => {
-                reject(new ToolkitError(`Failed to fetch notices: ${formatErrorMessage(e)}`));
+                reject(ToolkitError.withCause(formatErrorMessage(e), e));
               });
             } else {
-              reject(new ToolkitError(`Failed to fetch notices. Status code: ${res.statusCode}`));
+              reject(new ToolkitError(`${humanHttpStatusError(res.statusCode!)} (Status code: ${res.statusCode})`));
             }
           });
-        req.on('error', reject);
+        req.on('error', e => {
+          reject(ToolkitError.withCause(humanNetworkError(e), e));
+        });
       } catch (e: any) {
-        reject(new ToolkitError(`HTTPS 'get' call threw an error: ${formatErrorMessage(e)}`));
+        reject(ToolkitError.withCause(formatErrorMessage(e), e));
       }
     });
 

packages/@aws-cdk/toolkit-lib/lib/util/index.ts

@@ -7,6 +7,7 @@ export * from './content-hash';
 export * from './directories';
 export * from './format-error';
 export * from './json';
+export * from './net';
 export * from './objects';
 export * from './parallel';
 export * from './package-info';

packages/@aws-cdk/toolkit-lib/lib/util/net.ts

@@ -0,0 +1,56 @@
+/**
+ * Get a human-readable error message for a network error
+ * @param error The network error object
+ */
+export function humanNetworkError(error: NodeJS.ErrnoException): string {
+  switch (error.code) {
+    case 'ENOTFOUND':
+      return `Cannot reach the server. Please check your internet connection or the URL (${(error as any).hostname}).`;
+    case 'ECONNREFUSED':
+      return `Connection refused. The server at ${(error as any).address}:${(error as any).port} is not accepting connections.`;
+    case 'ECONNRESET':
+      return 'Connection was suddenly closed by the server. Please try again later.';
+    case 'ETIMEDOUT':
+      return 'Connection timed out. The server took too long to respond.';
+    case 'CERT_HAS_EXPIRED':
+      return 'The SSL certificate of the server has expired. This could be a security risk.';
+    case 'UNABLE_TO_VERIFY_LEAF_SIGNATURE':
+    case 'CERT_SIGNATURE_FAILURE':
+    case 'ERR_TLS_CERT_ALTNAME_INVALID':
+      return 'SSL certificate validation failed. This could indicate a security issue or a misconfigured server.';
+    default:
+      return `Network error: ${error.message || error.code || 'Unknown error'}`;
+  }
+}
+
+/**
+ * Get a human-readable error message for a HTTP status code
+ */
+export function humanHttpStatusError(statusCode: number): string {
+  switch (statusCode) {
+    case 400:
+      return 'Bad request - the server could not understand the request';
+    case 401:
+      return 'Unauthorized - authentication is required';
+    case 403:
+      return 'Forbidden - you do not have permission to access this resource';
+    case 404:
+      return 'Not found - the requested resource does not exist';
+    case 408:
+      return 'Request timeout - the server timed out waiting for the request';
+    case 429:
+      return 'Too many requests - you have sent too many requests in a given amount of time';
+    case 500:
+      return 'Internal server error - something went wrong on the server';
+    case 502:
+      return 'Bad gateway - the server received an invalid response from an upstream server';
+    case 503:
+      return 'Service unavailable - the server is temporarily unable to handle the request';
+    case 504:
+      return 'Gateway timeout - the server did not receive a timely response from an upstream server';
+    default:
+      return statusCode >= 500
+        ? 'Server error - something went wrong on the server'
+        : 'Client error - something went wrong with the request';
+  }
+}

packages/@aws-cdk/toolkit-lib/test/api/environment/environment-resources.test.ts

@@ -109,7 +109,7 @@ describe('validateversion without bootstrap stack', () => {
     await expect(envResources().validateVersion(8, '/abc')).resolves.toBeUndefined();
 
     const filter = jest.spyOn(NoticesFilter.prototype, 'filter');
-    notices.display();
+    await notices.display();
 
     expect(filter).toHaveBeenCalledTimes(1);
     expect(filter).toHaveBeenCalledWith({