From 52e737b2a9efccee550f23e45660946a3ef9fa59 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Thu, 14 Sep 2023 12:23:05 -0700 Subject: [PATCH 1/2] Drop OBJECTSTORAGE_KEY and SERVER_BODYLIMIT configurations In our case, json parsing for arguments do not require a body larger than the default library length of 100kb, so we are removing the configuration option entirely. Signed-off-by: Jeremy Ho --- .github/actions/deploy-to-environment/action.yaml | 1 - .github/environments/values.dev.yaml | 1 - .github/environments/values.prod.yaml | 1 - .github/environments/values.test.yaml | 1 - app/app.js | 2 +- app/config/custom-environment-variables.json | 1 - app/config/default.json | 1 - charts/coms/Chart.yaml | 2 +- charts/coms/README.md | 4 ++-- charts/coms/values.yaml | 1 - 10 files changed, 4 insertions(+), 11 deletions(-) diff --git a/.github/actions/deploy-to-environment/action.yaml b/.github/actions/deploy-to-environment/action.yaml index 3b8e4ca2..d014fa9c 100644 --- a/.github/actions/deploy-to-environment/action.yaml +++ b/.github/actions/deploy-to-environment/action.yaml @@ -50,7 +50,6 @@ runs: --set image.repository=ghcr.io/${{ github.repository_owner }} --set image.tag=sha-$(git rev-parse --short HEAD) --set route.host=${{ inputs.acronym }}-${{ inputs.namespace_environment }}-${{ inputs.job_name }}.apps.silver.devops.gov.bc.ca - --set config.configMap.OBJECTSTORAGE_KEY=${{ inputs.acronym }}/${{ inputs.namespace_environment }} --timeout 10m --wait diff --git a/.github/environments/values.dev.yaml b/.github/environments/values.dev.yaml index 615762c5..51db9605 100644 --- a/.github/environments/values.dev.yaml +++ b/.github/environments/values.dev.yaml @@ -19,7 +19,6 @@ config: MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuy7zfh2ZgpDV5mH/aXyLDTddZK81rGakJcTy4KvCNOkDDxt1KAhW02lmbCo8YhHCOzjNZBp1+Vi6QiMRgBqAe2GTPZYEiV70aXfROGZe3Nvwcjbtki6HoyRte3SpqLJEIPL2F+hjJkw1UPGnjPTWZkEx9p74b9i3BjuE8RnjJ0Sza2MWw83zoQUZEJRGiopSL0yuVej6t2LO2btVdVf7QuZfPt9ehkcQYlPKpVvJA+pfeqPAdnNt7OjEIeYxinjurZr8Z04hz8UhkRefcWlSbFzFQYmL7O7iArjW0bsSvq8yNUd5r0KCOQkFduwZy26yTzTxj8OLFT91fEmbBBl4rQIDAQAB KC_REALM: standard KC_SERVERURL: "https://dev.loginproxy.gov.bc.ca/auth" - SERVER_BODYLIMIT: 30mb # SERVER_LOGFILE: ~ SERVER_LOGLEVEL: http SERVER_PORT: "3000" diff --git a/.github/environments/values.prod.yaml b/.github/environments/values.prod.yaml index 9a4270e9..c04fbf2e 100644 --- a/.github/environments/values.prod.yaml +++ b/.github/environments/values.prod.yaml @@ -19,7 +19,6 @@ config: MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmHiuPKOkpkq4GXN1ktr23rJtDl6Vdu/Y37ZAd3PnQ8/IDfAODvy1Y81aAUZicKe9egolv+OTRANN3yOg+TAbRhkeXLE5p/473EK0aQ0NazTCuWo6Am3oDQ7Yt8x0pw56/qcLtkTuXNyo5EnVV2Z2BzCnnaL31JOhyitolku0DNT6GDoRBmT4o2ItqEVHk5nM25cf1t2zbwI2790W6if1B2qVRkxxivS8tbH7nYC61Is3XCPockKptkH22cm2ZQJmtYd5sZKuXaGsvtyzHmn8/l0Kd1xnHmUu4JNuQ67YiNZGu3hOkrF0Js3BzAk1Qm4kvYRaxbJFCs/qokLZ4Z0W9wIDAQAB KC_REALM: standard KC_SERVERURL: "https://loginproxy.gov.bc.ca/auth" - SERVER_BODYLIMIT: 30mb # SERVER_LOGFILE: ~ SERVER_LOGLEVEL: http SERVER_PORT: "3000" diff --git a/.github/environments/values.test.yaml b/.github/environments/values.test.yaml index 71e1386d..859154b1 100644 --- a/.github/environments/values.test.yaml +++ b/.github/environments/values.test.yaml @@ -19,7 +19,6 @@ config: MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAiFdv9GA83uHuy8Eu9yiZHGGF9j6J8t7FkbcpaN81GDjwbjsIJ0OJO9dKRAx6BAtTC4ubJTBJMPvQER5ikOhIeBi4o25fg61jpgsU6oRZHkCXc9gX6mrjMjbsPaf3/bjjYxP5jicBDJQeD1oRa24+tiGggoQ7k6gDEN+cRYqqNpzC/GQbkUPk8YsgroncEgu8ChMh/3ERsLV2zorchMANUq76max16mHrhtWIQxrb/STpSt4JuSlUzzBV/dcXjJe5gywZHe0jAutFhNqjHzHdgyaC4RAd3eYQo+Kl/JOgy2AZrnx+CiPmvOJKe9tAW4k4H087ng8aVE40v4HW/FEbnwIDAQAB KC_REALM: standard KC_SERVERURL: "https://test.loginproxy.gov.bc.ca/auth" - SERVER_BODYLIMIT: 30mb # SERVER_LOGFILE: ~ SERVER_LOGLEVEL: http SERVER_PORT: "3000" diff --git a/app/app.js b/app/app.js index b03d818b..95f82d2d 100644 --- a/app/app.js +++ b/app/app.js @@ -31,7 +31,7 @@ let probeId; let queueId; const app = express(); -const jsonParser = express.json({ limit: config.get('server.bodyLimit') }); +const jsonParser = express.json(); jsonParser.unless = unless; app.use(compression()); app.use(cors(DEFAULTCORS)); diff --git a/app/config/custom-environment-variables.json b/app/config/custom-environment-variables.json index 763b500e..961352fc 100644 --- a/app/config/custom-environment-variables.json +++ b/app/config/custom-environment-variables.json @@ -31,7 +31,6 @@ "secretAccessKey": "OBJECTSTORAGE_SECRETACCESSKEY" }, "server": { - "bodyLimit": "SERVER_BODYLIMIT", "defaultTempExpiresIn": "SERVER_TEMP_EXPIRESIN", "hardReset": "SERVER_HARDRESET", "logFile": "SERVER_LOGFILE", diff --git a/app/config/default.json b/app/config/default.json index ff1eaae4..0f2ccbc9 100644 --- a/app/config/default.json +++ b/app/config/default.json @@ -8,7 +8,6 @@ "username": "app" }, "server": { - "bodyLimit": "30mb", "defaultTempExpiresIn": "300", "logLevel": "http", "maxRetries": "3", diff --git a/charts/coms/Chart.yaml b/charts/coms/Chart.yaml index b839d59a..8559d3a0 100644 --- a/charts/coms/Chart.yaml +++ b/charts/coms/Chart.yaml @@ -3,7 +3,7 @@ name: common-object-management-service # This is the chart version. This version number should be incremented each time you make changes # to the chart and its templates, including the app version. # Versions are expected to follow Semantic Versioning (https://semver.org/) -version: 0.0.18 +version: 0.0.19 kubeVersion: ">= 1.13.0" description: A microservice for managing access control to S3 Objects # A chart can be either an 'application' or a 'library' chart. diff --git a/charts/coms/README.md b/charts/coms/README.md index c8b5f5ce..554d7749 100644 --- a/charts/coms/README.md +++ b/charts/coms/README.md @@ -1,6 +1,6 @@ # common-object-management-service -![Version: 0.0.18](https://img.shields.io/badge/Version-0.0.18-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.6.0](https://img.shields.io/badge/AppVersion-0.6.0-informational?style=flat-square) +![Version: 0.0.19](https://img.shields.io/badge/Version-0.0.19-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.6.0](https://img.shields.io/badge/AppVersion-0.6.0-informational?style=flat-square) A microservice for managing access control to S3 Objects @@ -35,7 +35,7 @@ Kubernetes: `>= 1.13.0` | autoscaling.targetCPUUtilizationPercentage | int | `80` | | | basicAuthSecretOverride.password | string | `nil` | | | basicAuthSecretOverride.username | string | `nil` | | -| config.configMap | object | `{"DB_PORT":"5432","KC_IDENTITYKEY":null,"KC_PUBLICKEY":null,"KC_REALM":null,"KC_SERVERURL":null,"OBJECTSTORAGE_BUCKET":null,"OBJECTSTORAGE_ENDPOINT":null,"OBJECTSTORAGE_KEY":null,"SERVER_BODYLIMIT":"30mb","SERVER_LOGLEVEL":"http","SERVER_PORT":"3000","SERVER_TEMP_EXPIRESIN":"300"}` | These values will be wholesale added to the configmap as is; refer to the coms documentation for what each of these values mean and whether you need them defined. Ensure that all values are represented explicitly as strings, as non-string values will not translate over as expected into container environment variables. For configuration keys named `*_ENABLED`, either leave them commented/undefined, or set them to string value "true". | +| config.configMap | object | `{"DB_PORT":"5432","KC_IDENTITYKEY":null,"KC_PUBLICKEY":null,"KC_REALM":null,"KC_SERVERURL":null,"OBJECTSTORAGE_BUCKET":null,"OBJECTSTORAGE_ENDPOINT":null,"OBJECTSTORAGE_KEY":null,"SERVER_LOGLEVEL":"http","SERVER_PORT":"3000","SERVER_TEMP_EXPIRESIN":"300"}` | These values will be wholesale added to the configmap as is; refer to the coms documentation for what each of these values mean and whether you need them defined. Ensure that all values are represented explicitly as strings, as non-string values will not translate over as expected into container environment variables. For configuration keys named `*_ENABLED`, either leave them commented/undefined, or set them to string value "true". | | config.enabled | bool | `false` | | | config.releaseScoped | bool | `false` | This should be set to true if and only if you require configmaps and secrets to be release scoped. In the event you want all instances in the same namespace to share a similar configuration, this should be set to false | | dbSecretOverride.password | string | `nil` | | diff --git a/charts/coms/values.yaml b/charts/coms/values.yaml index b0b96382..7b90f76d 100644 --- a/charts/coms/values.yaml +++ b/charts/coms/values.yaml @@ -145,7 +145,6 @@ config: OBJECTSTORAGE_ENDPOINT: ~ OBJECTSTORAGE_KEY: ~ - SERVER_BODYLIMIT: "30mb" # SERVER_HARDRESET: "true" # SERVER_LOGFILE: ~ SERVER_LOGLEVEL: "http" From 806f7fcb6558544c186831198e44ba2222ad6050 Mon Sep 17 00:00:00 2001 From: Jeremy Ho Date: Thu, 14 Sep 2023 12:32:18 -0700 Subject: [PATCH 2/2] Refactor express.json to only be used on necessary routes The express-unless library ended up being a cognitive risk to easily understanding when a json body needs to be parsed and under which routes and conditions. By modifying our router logic to only execute express.json() when the endpoint really needs it, we can avoid this problem and improve code intent and clarity. Signed-off-by: Jeremy Ho --- app/app.js | 10 ---------- app/package-lock.json | 11 ----------- app/package.json | 1 - app/src/routes/v1/bucket.js | 7 ++++--- app/src/routes/v1/permission/bucketPermission.js | 5 +++-- app/src/routes/v1/permission/objectPermission.js | 5 +++-- 6 files changed, 10 insertions(+), 29 deletions(-) diff --git a/app/app.js b/app/app.js index 95f82d2d..0d085236 100644 --- a/app/app.js +++ b/app/app.js @@ -3,7 +3,6 @@ const compression = require('compression'); const config = require('config'); const cors = require('cors'); const express = require('express'); -const { unless } = require('express-unless'); const { ValidationError } = require('express-validation'); const { AuthMode, DEFAULTCORS } = require('./src/components/constants'); @@ -31,17 +30,8 @@ let probeId; let queueId; const app = express(); -const jsonParser = express.json(); -jsonParser.unless = unless; app.use(compression()); app.use(cors(DEFAULTCORS)); -app.use(jsonParser.unless({ - path: [{ - // Matches on only the createObject and updateObject endpoints - url: /.*(? { +router.put('/', express.json(), bucketValidator.createBucket, (req, res, next) => { bucketController.createBucket(req, res, next); }); @@ -35,7 +36,7 @@ router.get('/', bucketValidator.searchBuckets, (req, res, next) => { }); /** Updates a bucket */ -router.patch('/:bucketId', bucketValidator.updateBucket, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.patch('/:bucketId', express.json(), bucketValidator.updateBucket, hasPermission(Permissions.UPDATE), (req, res, next) => { bucketController.updateBucket(req, res, next); }); diff --git a/app/src/routes/v1/permission/bucketPermission.js b/app/src/routes/v1/permission/bucketPermission.js index 8a52a743..765d572f 100644 --- a/app/src/routes/v1/permission/bucketPermission.js +++ b/app/src/routes/v1/permission/bucketPermission.js @@ -1,4 +1,5 @@ -const router = require('express').Router(); +const express = require('express'); +const router = express.Router(); const { Permissions } = require('../../../components/constants'); const { bucketPermissionController } = require('../../../controllers'); @@ -20,7 +21,7 @@ router.get('/:bucketId', bucketPermissionValidator.listPermissions, currentObjec }); /** Grants bucket permissions to users */ -router.put('/:bucketId', bucketPermissionValidator.addPermissions, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { +router.put('/:bucketId', express.json(), bucketPermissionValidator.addPermissions, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { bucketPermissionController.addPermissions(req, res, next); }); diff --git a/app/src/routes/v1/permission/objectPermission.js b/app/src/routes/v1/permission/objectPermission.js index a8277321..431e5c05 100644 --- a/app/src/routes/v1/permission/objectPermission.js +++ b/app/src/routes/v1/permission/objectPermission.js @@ -1,4 +1,5 @@ -const router = require('express').Router(); +const express = require('express'); +const router = express.Router(); const { Permissions } = require('../../../components/constants'); const { objectPermissionController } = require('../../../controllers'); @@ -20,7 +21,7 @@ router.get('/:objectId', objectPermissionValidator.listPermissions, currentObjec }); /** Grants object permissions to users */ -router.put('/:objectId', objectPermissionValidator.addPermissions, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { +router.put('/:objectId', express.json(), objectPermissionValidator.addPermissions, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { objectPermissionController.addPermissions(req, res, next); });