Merge branch 'main' into test/forms-1371

.devcontainer/chefs_local/local.json.sample

@@ -53,8 +53,9 @@
       "port": "8080",
       "rateLimit" : {
         "public": {
-          "windowMs": "900000",
-          "max": "100"
+          "limitApiKey": "120",
+          "limitFrontend": "500",
+          "windowMs": "60000",
         }
       },
       "encryption": {

app/config/default.json

@@ -53,8 +53,9 @@
     "port": "8080",
     "rateLimit": {
       "public": {
-        "windowMs": "60000",
-        "max": "120"
+        "limitApiKey": "120",
+        "limitFrontend": "500",
+        "windowMs": "60000"
       }
     },
     "encryption": {

app/package.json

@@ -61,7 +61,7 @@
     "cryptr": "^6.3.0",
     "express": "^4.21.0",
     "express-basic-auth": "^1.2.1",
-    "express-rate-limit": "^7.2.0",
+    "express-rate-limit": "^7.4.0",
     "express-winston": "^4.2.0",
     "falsey": "^1.0.0",
     "fs-extra": "^10.1.0",

app/src/forms/common/middleware/rateLimiter.js

@@ -1,14 +1,41 @@
 const config = require('config');
 const rateLimit = require('express-rate-limit');
 
+/**
+ * Returns true if the caller is using an API Key (Basic auth). Note that the
+ * web application will have no auth for unauthenticated (public) users, or
+ * Bearer auth for authenticated users.
+ *
+ * @param {string} authorizationHeader the HTTP request's authorization header.
+ * @returns a boolean that is true if the caller is using Basic auth.
+ */
+const _isApiKeyUser = (authorizationHeader) => {
+  return authorizationHeader && authorizationHeader.startsWith('Basic ');
+};
+
+/**
+ * Gets the rate limit to use depending on whether or not the call is using an
+ * API Key (Basic auth).
+ *
+ * @param {string} authorizationHeader the HTTP request's authorization header.
+ */
+const _getRateLimit = (authorizationHeader) => {
+  let rateLimit;
+
+  if (_isApiKeyUser(authorizationHeader)) {
+    rateLimit = config.get('server.rateLimit.public.limitApiKey');
+  } else {
+    rateLimit = config.get('server.rateLimit.public.limitFrontend');
+  }
+
+  return rateLimit;
+};
+
 const apiKeyRateLimiter = rateLimit({
   // Instead of legacy headers use the standardHeaders version defined below.
   legacyHeaders: false,
 
-  limit: config.get('server.rateLimit.public.max'),
-
-  // Skip everything except Basic auth so that CHEFS app users are not limited.
-  skip: (req) => !req.headers?.authorization || !req.headers.authorization.startsWith('Basic '),
+  limit: (req) => _getRateLimit(req.headers?.authorization),
 
   // Use the latest draft of the IETF standard for rate limiting headers.
   standardHeaders: 'draft-7',

app/tests/unit/README.md

@@ -56,31 +56,30 @@ Similar to `.only` is the `.skip` modifier to skip a test or group of tests.
 
 ## Testing Strategy
 
-The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests we should:
+The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests:
 
-- ensure that the tests are consistent
-- ensure that we have 100% test coverage
-- ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage
-- test the interface, not the implementation
-- test the unit under test, not its dependencies
+- Ensure that the tests are consistent: be sure to completely understand similar tests before creating a new test
+- Ensure that we have 100% test coverage
+- Ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage
+- Test the interface, not the implementation
+- Test the unit under test, not its dependencies
 
 ### Middleware Testing
 
-The tests for the middleware files should:
+The tests for the middleware files:
 
-- mock all services calls used by the middleware, including both exception and minimal valid results
-- test all response codes produced by the middleware
+- Mock all services calls used by the middleware, including both exception and minimal valid results
+- Test all response codes produced by the middleware
 
 ### Route Testing
 
-The tests for the `route.js` files should:
+The tests for the `route.js` files:
 
-- mock all middleware used by the file
-- each route test should check that every middleware is called the proper number of times
-- each route test should mock the controller function that it calls
-- mock controller functions with `res.sendStatus(200)`, as doing something like `next()` will call multiple controller functions when route paths are overloaded
-- check that the mocked controller function is called - this will catch when a new route path accidentally overloads an old one
-- for consistency and ease of comparison, alphabetize the expect clauses ("alphabetize when possible")
+- Mock middleware used by the file
+- Each route test mocks its controller function with `res.sendStatus(200)`, as doing something like `next()` calls multiple controller functions when route paths are overloaded
+- Check that the route calls every middleware the proper number of times - should be 0 or 1
+- Check that the route calls the expected controller function - this will catch when a new route path accidentally overloads an old one
+- Alphabetize the expect clauses ("alphabetize when possible") for consistency and ease of comparison
 
 Note:
 

app/tests/unit/forms/common/middleware/rateLimiter.spec.js

@@ -3,14 +3,19 @@ const uuid = require('uuid');
 
 const { apiKeyRateLimiter } = require('../../../../../src/forms/common/middleware');
 
-const rateLimit = 7;
+const rateLimitApiKey = 5;
+const rateLimitFrontend = 7;
 const rateWindowSeconds = 11;
 
 jest.mock('config', () => {
   return {
     get: jest.fn((key) => {
-      if (key === 'server.rateLimit.public.max') {
-        return rateLimit;
+      if (key === 'server.rateLimit.public.limitApiKey') {
+        return rateLimitApiKey;
+      }
+
+      if (key === 'server.rateLimit.public.limitFrontend') {
+        return rateLimitFrontend;
       }
 
       if (key === 'server.rateLimit.public.windowMs') {
@@ -22,9 +27,11 @@ jest.mock('config', () => {
 
 // Headers for Draft 7 of the standard.
 const rateLimitName = 'RateLimit';
-const rateLimitValue = `limit=${rateLimit}, remaining=${rateLimit - 1}, reset=${rateWindowSeconds}`;
+const rateLimitValueApiKey = `limit=${rateLimitApiKey}, remaining=${rateLimitApiKey - 1}, reset=${rateWindowSeconds}`;
+const rateLimitValueFrontend = `limit=${rateLimitFrontend}, remaining=${rateLimitFrontend - 1}, reset=${rateWindowSeconds}`;
 const rateLimitPolicyName = 'RateLimit-Policy';
-const rateLimitPolicyValue = `${rateLimit};w=${rateWindowSeconds}`;
+const rateLimitPolicyValueApiKey = `${rateLimitApiKey};w=${rateWindowSeconds}`;
+const rateLimitPolicyValueFrontend = `${rateLimitFrontend};w=${rateWindowSeconds}`;
 
 const ipAddress = '1.2.3.4';
 
@@ -54,8 +61,8 @@ describe('apiKeyRateLimiter', () => {
 
     expect(res.setHeader).toBeCalledTimes(2);
     // These also test that the rate limiter uses our custom config values.
-    expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValue);
-    expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValue);
+    expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueApiKey);
+    expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueApiKey);
     expect(next).toBeCalledTimes(1);
     expect(next).toBeCalledWith();
   });
@@ -70,7 +77,10 @@ describe('apiKeyRateLimiter', () => {
 
       await apiKeyRateLimiter(req, res, next);
 
-      expect(res.setHeader).toBeCalledTimes(0);
+      expect(res.setHeader).toBeCalledTimes(2);
+      // These also test that the rate limiter uses our custom config values.
+      expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend);
+      expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend);
       expect(next).toBeCalledTimes(1);
       expect(next).toBeCalledWith();
     });
@@ -85,7 +95,10 @@ describe('apiKeyRateLimiter', () => {
 
       await apiKeyRateLimiter(req, res, next);
 
-      expect(res.setHeader).toBeCalledTimes(0);
+      expect(res.setHeader).toBeCalledTimes(2);
+      // These also test that the rate limiter uses our custom config values.
+      expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend);
+      expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend);
       expect(next).toBeCalledTimes(1);
       expect(next).toBeCalledWith();
     });
@@ -102,7 +115,10 @@ describe('apiKeyRateLimiter', () => {
 
       await apiKeyRateLimiter(req, res, next);
 
-      expect(res.setHeader).toBeCalledTimes(0);
+      expect(res.setHeader).toBeCalledTimes(2);
+      // These also test that the rate limiter uses our custom config values.
+      expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend);
+      expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend);
       expect(next).toBeCalledTimes(1);
       expect(next).toBeCalledWith();
     });
@@ -119,7 +135,10 @@ describe('apiKeyRateLimiter', () => {
 
       await apiKeyRateLimiter(req, res, next);
 
-      expect(res.setHeader).toBeCalledTimes(0);
+      expect(res.setHeader).toBeCalledTimes(2);
+      // These also test that the rate limiter uses our custom config values.
+      expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend);
+      expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend);
       expect(next).toBeCalledTimes(1);
       expect(next).toBeCalledWith();
     });
@@ -136,7 +155,10 @@ describe('apiKeyRateLimiter', () => {
 
       await apiKeyRateLimiter(req, res, next);
 
-      expect(res.setHeader).toBeCalledTimes(0);
+      expect(res.setHeader).toBeCalledTimes(2);
+      // These also test that the rate limiter uses our custom config values.
+      expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend);
+      expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend);
       expect(next).toBeCalledTimes(1);
       expect(next).toBeCalledWith();
     });

openshift/README.md

@@ -241,7 +241,7 @@ Refer to the `patroni.dc.yaml` and `patroni.secret.yaml` files found below for a
 
 After backups are made a verification job should be run to restore the backup into a temporary database and check that tables are created and data is written. This is not a full verification to ensure all data integrity, but it is an automatable first step.
 
-Using the `backup-cronjon-verify.yaml` file from the `redash` directory:
+Using the `backup-cronjon-verify.yaml` file from the `redash` directory we will use the default resource values for the Development and Test Environments:
 
 ```sh
 export NAMESPACE=<yournamespace>
@@ -255,6 +255,26 @@ oc process -f backup-cronjob-verify.yaml \
     -p JOB_NAME=backup-postgres-verify \
     -p JOB_PERSISTENT_STORAGE_NAME=$PVC \
     -p SCHEDULE="0 9 * * *" \
-    -p TAG_NAME=2.8.0 \
+    -p TAG_NAME=2.9.0 \
+    | oc -n $NAMESPACE apply -f -
+```
+
+For the Production environment we have a much larger database and we want the verification to complete within a reasonable amount of time. Increase the limits so that the container is not memory- or cpu-bound:
+
+```sh
+export NAMESPACE=<yournamespace>
+export PVC=<yourpvcname>
+
+oc process -f backup-cronjob-verify.yaml \
+    -p BACKUP_JOB_CONFIG=backup-postgres-config \
+    -p DATABASE_DEPLOYMENT_NAME=patroni-master-secret \
+    -p DATABASE_PASSWORD_KEY_NAME=app-db-password \
+    -p DATABASE_USER_KEY_NAME=app-db-username \
+    -p JOB_NAME=backup-postgres-verify \
+    -p JOB_PERSISTENT_STORAGE_NAME=$PVC \
+    -p RESOURCE_LIMIT_CPU="1500m" \
+    -p RESOURCE_LIMIT_MEMORY="1Gi" \
+    -p SCHEDULE="0 9 * * *" \
+    -p TAG_NAME=2.9.0 \
     | oc -n $NAMESPACE apply -f -
 ```

openshift/redash/README.md

@@ -26,7 +26,7 @@ oc process -f backup-cronjob.yaml \
     -p JOB_PERSISTENT_STORAGE_NAME=backup-chefs-redash-postgresql \
     -p MONTHLY_BACKUPS=3 \
     -p SCHEDULE="0 8 * * *" \
-    -p TAG_NAME=2.8.0 \
+    -p TAG_NAME=2.9.0 \
     -p WEEKLY_BACKUPS=8 \
     | oc -n a12c97-tools apply -f -
 ```
@@ -42,7 +42,7 @@ oc process -f backup-cronjob-verify.yaml \
     -p JOB_NAME=backup-chefs-redash-postgres-verify \
     -p JOB_PERSISTENT_STORAGE_NAME=backup-chefs-redash-postgresql \
     -p SCHEDULE="0 9 * * *" \
-    -p TAG_NAME=2.8.0 \
+    -p TAG_NAME=2.9.0 \
     | oc -n a12c97-tools apply -f -
 ```
 

openshift/redash/backup-cronjob-verify.yaml

@@ -22,6 +22,26 @@ parameters:
     description: "Pre-Created PVC to use for backup target"
     value: "bk-devex-von-tools-a9vlgd1jpsg1"
     required: true
+  - name: "RESOURCE_LIMIT_CPU"
+    displayName: "CPU Limit"
+    description: "The maximium CPU resources for the container"
+    value: "250m"
+    required: true
+  - name: "RESOURCE_LIMIT_MEMORY"
+    displayName: "Memory Limit"
+    description: "The maximium memory resources for the container"
+    value: "1Gi"
+    required: true
+  - name: "RESOURCE_REQUEST_CPU"
+    displayName: "CPU Request"
+    description: "The minimium CPU resources for the container"
+    value: "50m"
+    required: true
+  - name: "RESOURCE_REQUEST_MEMORY"
+    displayName: "Memory Request"
+    description: "The minimium memory resources for the container"
+    value: "256Mi"
+    required: true
   - name: "SCHEDULE"
     displayName: "Cron Schedule"
     description: "Cron Schedule to Execute the Job (using local cluster system TZ)"
@@ -161,12 +181,19 @@ objects:
             spec:
               containers:
                 - name: "${JOB_NAME}-cronjob"
+                  resources:
+                    limits:
+                      cpu: "${RESOURCE_LIMIT_CPU}"
+                      memory: "${RESOURCE_LIMIT_MEMORY}"
+                    requests:
+                      cpu: "${RESOURCE_REQUEST_CPU}"
+                      memory: "${RESOURCE_REQUEST_MEMORY}"
                   image: "${IMAGE_REGISTRY}/${IMAGE_NAMESPACE}/${SOURCE_IMAGE_NAME}:${TAG_NAME}"
                   # image: backup
                   command:
                     - "/bin/bash"
                     - "-c"
-                    - "/backup.sh -v all"
+                    - "/backup.sh -I -v all"
                   volumeMounts:
                     - mountPath: "${BACKUP_DIR}"
                       name: "backup"