Skip to content

Commit

Permalink
Merge branch 'main' into test/forms-1371
Browse files Browse the repository at this point in the history
  • Loading branch information
nimya-aot authored Sep 26, 2024
2 parents 9413294 + 67d7aae commit 6da9431
Show file tree
Hide file tree
Showing 10 changed files with 146 additions and 49 deletions.
5 changes: 3 additions & 2 deletions .devcontainer/chefs_local/local.json.sample
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@
"port": "8080",
"rateLimit" : {
"public": {
"windowMs": "900000",
"max": "100"
"limitApiKey": "120",
"limitFrontend": "500",
"windowMs": "60000",
}
},
"encryption": {
Expand Down
5 changes: 3 additions & 2 deletions app/config/default.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@
"port": "8080",
"rateLimit": {
"public": {
"windowMs": "60000",
"max": "120"
"limitApiKey": "120",
"limitFrontend": "500",
"windowMs": "60000"
}
},
"encryption": {
Expand Down
14 changes: 7 additions & 7 deletions app/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
35 changes: 31 additions & 4 deletions app/src/forms/common/middleware/rateLimiter.js
Original file line number Diff line number Diff line change
@@ -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',
Expand Down
31 changes: 15 additions & 16 deletions app/tests/unit/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
46 changes: 34 additions & 12 deletions app/tests/unit/forms/common/middleware/rateLimiter.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand All @@ -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';

Expand Down Expand Up @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand Down
24 changes: 22 additions & 2 deletions openshift/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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 -
```
4 changes: 2 additions & 2 deletions openshift/redash/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 -
```
Expand All @@ -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 -
```

Expand Down
29 changes: 28 additions & 1 deletion openshift/redash/backup-cronjob-verify.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 6da9431

Please sign in to comment.