CUMULUS-3433 - Update to nodeJs v20 #3636

Jkovarik · 2024-04-25T21:22:12Z

Summary: Summary of changes

Addresses CUMULUS-3433: Develop amazing new feature

Changes

CUMULUS-3433
- Added importGot helper method to import got as an ESM module in
  CommmonJS typescript/webpack clients.
CUMULUS-3433
- Updated all node.js lambda dependencies to node 20.x/20.12.2
- Modified @cumulus/ingest unit test HTTPs server to accept localhost POST
  requests, and removed nock dependency from tests involving fs.Readstream
  and got due to a likely incompatibility with changes in node v18, got,
  fs.Readstream and nock when used in combination in units
  (POSTs whose bodys are instances of createReadStream hang with latest node sindresorhus/got#2341)

Reviewers, please note this PR is still in-work in that the following need to occur:

CMA-JS PR needs to pass/be published for the cma-js dependency updates to be added
- Update cma-js to build/run under node 20 cumulus-message-adapter-js#69
cumuluss/async-operation needs to be published (after review sign-off) prior to release

PR Checklist

Update CHANGELOG
Unit tests
Ad-hoc testing - Deploy changes and test manually
Integration tests

.sort is intermittently failing to order as expected, causing this test to fail in local test at an alarming failure rate. Updating test to be more explicit/remove sort, however this needs to be investigated prior to closing thie branch/PR/ticket

JSON.parse now throws a different error for the merged test case.

error

This reverts commit adbad84.

This reverts commit a19f9d1.

This reverts commit e652516.

This update has a couple of controversial changes in it: Updating got to v14 means we're using a pure ESM module given sindre's stance on not supporting common exports. That's being done due to incompatible changes in node streams requires at least got v12 Additionally there's a probable outstanding issue in got sindresorhus/got#2341 related to node v20/fs readstreams/nock and/or msw incompatibility (as well as a possible open source contrib)

Updating the existing apache server to return 200 on an existing endpoint is far better than the prior commit approach in that it's a valid/useful unit test as a result, with the technical/less tidy downside of requiring the unit test stack to be working.

…host

Jkovarik · 2024-04-25T21:22:48Z

bamboo/docker-compose-local.yml

-      - 5432:5432
-      - 8080:8080
-      - 9200:9200
+      - "127.0.0.1:20:20"


This is a security update to disallow port access from hosts other than localhost.

Jkovarik · 2024-04-25T21:23:22Z

bamboo/httpd.conf

@@ -388,6 +389,15 @@ LogLevel warn
    #Scriptsock cgisock
 </IfModule>

+
+### Configuration to allow PUT endpoint


Updates Apache server to have a dummy response to HTTP POST

Jkovarik · 2024-04-25T21:23:54Z

example/cumulus-tf/variables.tf

@@ -350,7 +350,7 @@ variable "rds_admin_access_secret_arn" {
 variable "async_operation_image_version" {
  description = "docker image version to use for Cumulus async operations tasks"
  type = string
-  default = "48"
+  default = "49"


Test image from changes in this PR was pushed up to v49 in sandbox

Jkovarik · 2024-04-25T21:25:15Z

packages/db/tests/test-connection.js

-    ]
-  );
+  t.deepEqual(loggerWarnStub.args[0][0], 'knex failed on attempted connection');
+  t.true(loggerWarnStub.args[0][1].errors.map((e) => e.message).includes('connect ECONNREFUSED 127.0.0.1:5400'));


This was changed as the underlying modules changed behaviors in the update and this can result in an aggregate error with more values than the original test.

etcart · 2024-04-26T16:08:27Z

bamboo/Dockerfile

@@ -1,6 +1,6 @@
 ## Dockerfile to create integration/unit test environment
-FROM node:16.19.0-buster
-RUN apt update && npm config set unsafe-perm true &&\


cool to see unsafe-perm go away here and elsewhere... what was it doing before that's changed?

NPM changed how it's invoking things in v7, instead of using the uid specified by the user config (nobody in the case of root) it uses the permissions from the folder, which works just fine for our use case.

Also, notably, they dropped support for this option when those changes were made.

etcart · 2024-04-26T16:08:51Z

bamboo/docker-compose-local.yml

@@ -9,20 +9,20 @@ services:
      - ../packages/test-data/keys/ssh_client_rsa_key.pub:/ssh_client_rsa_key.pub
      - ../packages/test-data:/data
  build_env:
-    image: cumuluss/cumulus-build-env:latest
+    image: cumuluss/cumulus-build-env:test


is this a leftover from testing?

Good catch - yes, I'll revert it 👍🏻

packages/api/tests/lambdas/test-kinesis-consumer.js

etcart · 2024-04-26T16:14:05Z

packages/common/importGot.js

what's going on with this? won't we just have to call this as await? is there more going on here than aliasing default to whatever we wanna call it?

Yes - this is needed because:

Compiling javascript into a CommonJS typescript module will not allow for dynamic imports. In the use case in @cumulus/ingest we don't want to make all of the JS not TS compiled as that doesn't generate typemaps and makes downstream TS module users define default/types.

If we wrap the ESM import statement in something like eval, that works great (tsc doesn't transpile it), however any user of the module that webpacks a lambda using it won't include the dependency (as using a hack like eval in this case confuses static dependency mapping).

Exporting it from a package as un-compiled JS allows us to:

Have a helper that imports got in this way to avoid the above issues in typescript modules with allowJs enabled

Avoid any downstream dependency detection issues

Handle this issue in a tidy way across the project

packages/ingest/test/test-HttpProviderClient.js

etcart · 2024-04-26T16:20:25Z

tasks/send-pan/tests/index.js

  // Message should look like this:
  // MESSAGE_TYPE = "SHORTPAN";
  // DISPOSITION = "SUCCESSFUL";
  // TIME_STAMP = 2023-03-27T18:10:56.402Z;
-  nock(url).post(remotePath, regex)


is this validation happening somewhere else?

Yes, and no. This is one of those cases where we're re-validating library behavior that itself mocks.

got is broken in node20 when used with nock/aws and file readstreams - and other options (axios) don't handle file streaming (as well/at all) . As such, this test is only testing that something attempts to post to the nock'd HTTP endpoint, which is unit tested in the ingest module already via the workaround noted in test-HttpProviderClient.

Opinion:
In light of the issues with nock/etc, got and node 20 the value here of requiring this is at least somewhat debatable (or at least pushes the bounds of social/unsocial units) given the code being covered by the unit isn't in this task, and we have coverage re: a PAN being sent. Removing this assertion doesn't directly reduce code coverage.

The alternate approaches include:

Update the HTTP server with a CGI script to actually take any post and write it to the container file system/serve it to validate something is posted and evaluate that response in the unit

Update the existing integration test to use a HTTP provider. This would also require an HTTP server that will take POST/put

Exposing the test mock params through the lambda client to this unit test

Fix got, or implement an alternative to utilize our test mocking structure.

Co-authored-by: etcart <[email protected]>

packages/common/importGot.js

etcart

talked through the details of importGot and cleared up the last confusion

…/cumulus into jk/CUMULUS-3433-update-node

Jkovarik · 2024-04-26T19:39:38Z

@etcart per our pairing/discussion session I added 2ff5403 - take a look and let me know what you think!

Jkovarik added 15 commits April 16, 2024 13:03

Update core deps to v20.12.2

4591a08

Update unit test to rmove .sort

adbad84

.sort is intermittently failing to order as expected, causing this test to fail in local test at an alarming failure rate. Updating test to be more explicit/remove sort, however this needs to be investigated prior to closing thie branch/PR/ticket

Update kinesis test to node v20

c0af32b

JSON.parse now throws a different error for the merged test case.

Update knex test to handle new subdependency throwing an Aggregate

dc0a7fd

error

Revert "Update unit test to rmove .sort"

7ae3588

This reverts commit adbad84.

Update got

a19f9d1

Revert "Update got"

e652516

This reverts commit a19f9d1.

Upgrade ava/nock, fix ingest provider tests

2a82ed4

Reapply "Update got"

83d06dd

This reverts commit e652516.

Update local test stack configuration to restrict connection to local…

c0d5147

…host

Update send-pan test/dependencies

9f288ad

Update lambdas/async operation to deploy with node 20

af26ff0

Update common to export a helper to import ESM got module

cb56285

Jkovarik added the work in progress label Apr 25, 2024

Jkovarik commented Apr 25, 2024

View reviewed changes

Jkovarik added 5 commits April 25, 2024 15:28

Add docstring to importGot

5aa92f6

Remove unneeded imports

8289b91

Minor PR review fixes

92bb668

Update CHANGELOG

61ea4e2

Update CHANGELOG

11be52e

Jkovarik added the needs review label Apr 25, 2024

Jkovarik added 4 commits April 25, 2024 17:13

Reconfigure CI to not use unsafe perms modification

9271951

Fix broken package.json

dd8bb8f

Fix extran. dep issue

9300c6c

Make error message test less specific

b903d86

Update test to not rely on aggregate internal errors

2eb3b8d

Jkovarik removed the work in progress label Apr 26, 2024

Merge branch 'master' into jk/CUMULUS-3433-update-node

2cadc6c

etcart reviewed Apr 26, 2024

View reviewed changes

Update packages/ingest/test/test-HttpProviderClient.js

fffe15b

Co-authored-by: etcart <[email protected]>

etcart reviewed Apr 26, 2024

View reviewed changes

packages/common/importGot.js Outdated Show resolved Hide resolved

etcart approved these changes Apr 26, 2024

View reviewed changes

Jkovarik added 2 commits April 26, 2024 13:38

Move importGot -> importEsm, introduce static import definition

2ff5403

Merge branch 'jk/CUMULUS-3433-update-node' of https://github.com/nasa…

5c19a84

…/cumulus into jk/CUMULUS-3433-update-node

Jkovarik added 2 commits April 26, 2024 13:40

Fix inadvertant test commit

972b71b

Update CMA-js dependency to v2.2.0

b95e1b2

Jkovarik removed the needs review label Apr 29, 2024

Remove dynamic/frozen import

de3c0f3

etcart approved these changes Apr 30, 2024

View reviewed changes

Jkovarik merged commit 9feefbe into master Apr 30, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUMULUS-3433 - Update to nodeJs v20 #3636

CUMULUS-3433 - Update to nodeJs v20 #3636

Jkovarik commented Apr 25, 2024 •

edited

Loading

Jkovarik Apr 25, 2024

Jkovarik Apr 25, 2024

Jkovarik Apr 25, 2024

Jkovarik Apr 25, 2024

etcart Apr 26, 2024

Jkovarik Apr 26, 2024

etcart Apr 26, 2024

Jkovarik Apr 26, 2024

Jkovarik Apr 26, 2024

etcart Apr 26, 2024

Jkovarik Apr 26, 2024

etcart Apr 26, 2024

Jkovarik Apr 26, 2024 •

edited

Loading

etcart left a comment

Jkovarik commented Apr 26, 2024

CUMULUS-3433 - Update to nodeJs v20 #3636

CUMULUS-3433 - Update to nodeJs v20 #3636

Conversation

Jkovarik commented Apr 25, 2024 • edited Loading

Changes

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jkovarik Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

etcart left a comment

Choose a reason for hiding this comment

Jkovarik commented Apr 26, 2024

Jkovarik commented Apr 25, 2024 •

edited

Loading

Jkovarik Apr 26, 2024 •

edited

Loading