fix(cli): cdk notices ignores failures and calling cli with --no-notices makes an unexpected network call (#399)

Fixes #370 
Fixes #440

Previously when the `--no-notices` option was passed, the CLI would
still attempt to refresh notices in the background. This is unnecessary
and an unexpected behavior to users, especially when running the CLI in
environments with no internet access.

On the other hand, running the explicit `cdk notices` command did
previously not fail if the command failed to refresh notices. Instead
the command would pretend no notices are found and complete
successfully.

This PR updates the code to not refresh notices when notices are
disabled. Unless the explicit `cdk notices` command is called, which
will always refresh and now error if fetching notices fails. It also
removes a double refresh of the notices (which didn't have an effect
because they were cached, but still).

`cdk notices` with an error now looks like this:

<img width="775" alt="image"
src="https://github.com/user-attachments/assets/5c09263c-a775-49c9-8dcf-e4feeefb10ac"
/>

Before the command would incorrectly and silently succeed and not show
any notices:

<img width="118" alt="image"
src="https://github.com/user-attachments/assets/dd93c1bf-4b6a-4fa9-8c9a-1fbacccc0156"
/>

Only when run with the `-v` flag, it would hint at the failure:

```console
$ cdk notices -v
...
[12:38:29] Could not refresh notices: Error: getaddrinfo ENOTFOUND cli.cdk.dev-tools
```
---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

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({