ref(app): shift support flows into separate folder + ecs service #1269

seaerchin · 2024-04-04T03:36:41Z

NOTE: production deploy stuff is in #1285

Problem

At present, some internal flows trigger a huge spike in CPU usage, which will erroneously trigger an alarm. additionally, this CPU spike might cause a degradation on performance of the actual app.

Closes ISOM-811

Solution

use a separate ecs service to do it. we did not go with a sidecar container as was initially suggested. this is because ecs scaling is done via metrics and the most fine-grained metric we have at present (that is relevant) is ecs cpu usage by service. as the sidecar is still part of the service, this would cause the support container to trigger scaling (and still alarms) on the service as a whole, which doesn't solve the problem.
shift shared services out into /common -> this is to facilitate sharing of services between support and src. the new service listens on port 8082 - this is because on local, we want to expose both services ports to the host machine (hence cannot use same port).
added deployment step for staging only for support - added 2 more params to the aws_deploy workflow to support this. (as service + dockerfile is now different)
add task-def for support - we cannot reuse existing one because it's a new ecs service.

Breaking Changes

Yes - this PR contains breaking changes
- not user facing, but the existing forms will not work and will require an update to <stg | prod>-support-api/v2/infra/formsg

Tests

These tests have been done on staging, but the success has been checked only via email (ie, receive success email = good enough)

submit the staging site launch form
should receive an email regarding the dns records to update
submit the staging site creation form
should receive a success email
submit staging audit logs form
should receive success email
submit staging site link checker form
should have logs ("*link*" in isomer-infra or isomer (not entirely sure why, suspect it's because our project tag in infra repo is isomer-infra which leads to the tag being isomer-infra)
submit staging site repair form
should see success email

seaerchin · 2024-04-04T05:44:57Z

.aws/deploy/support-task-definition.staging.json

+      "logConfiguration": {
+        "logDriver": "awslogs",
+        "options": {
+          "awslogs-group": "/aws/elasticbeanstalk/cms-backend-staging-node18/var/log/web.stdout.log",


open to creating another log group, nbd

+1 to creating another log group

Maybe we can drop elasticbeanstalk from the name too...

ya can, i think i'll change it to an ecs-prefixed log group post tear down of eb

seaerchin · 2024-04-04T05:45:29Z

.aws/deploy/support-task-definition.staging.json

+      "name": "support",
+      "portMappings": [
+        {
+          "containerPort": 8082,


only change between this and backend

hmm why do we not hahve a stg variant of this ah?

this deploys to staging! i think u mean prod? will do in a folloow up PR

seaerchin · 2024-04-04T05:46:10Z

common/index.ts

no big changes, just copied out services from server.js -> here + changed from require to import

seaerchin · 2024-04-04T05:48:28Z

support/Dockerfile

+COPY ../src ./src
+COPY ../common ./common
+COPY ../support ./support
+COPY ../package.json . 
+COPY ../package-lock.json .
+COPY ../scripts ./scripts
+COPY ../tsconfig.json .


change 1 - we selectively copy rather than just COPY . .; we should be able to omit COPY ../src ./src if we are rigorous about separation of concerns but for now this is ok. build times are not impacted too much cos both jobs run in parallel

seaerchin · 2024-04-04T05:50:56Z

support/Dockerfile

+RUN echo "  name = Isomer Admin" >> /home/webapp/.gitconfig
+RUN echo "  email = [email protected]" >> /home/webapp/.gitconfig
+
+EXPOSE "8082"


need this because we using port 8082

kishore03109

lgtm, i felt that we should use the side car for more intensive operations only (eg site launch + create does not seem like intensive ops, but this is a nit)

kishore03109 · 2024-04-05T07:44:51Z

.aws/deploy/support-task-definition.staging.json

+      "name": "support",
+      "portMappings": [
+        {
+          "containerPort": 8082,


hmm why do we not hahve a stg variant of this ah?

kishore03109 · 2024-04-05T07:49:20Z

src/types/request.ts

-  res: Response<ResBody, Locals>,
-  next: NextFunction
-) => void
+> = ExpressHandler<P, ResBody, ReqBody, ReqQuery, Locals>


hmm context behind this change? did we type this wrongly ah?

the change was made to keep our types neater. we already have a global request.d.ts, which overrides the Request type exported originally!

this means that the original typing here is superfluous and hence, removed

kishore03109 · 2024-04-05T08:07:49Z

support/Dockerfile

+RUN adduser -u 900 webapp -D -h /home/webapp -s /bin/sh
+USER webapp
+
+COPY ../src ./src


if we are spiltting this up, should we split this up for the main docker file as well?

to clarify, we want to omit copying of support into our main Dockerfile?

we totally can honestly, will make the change. best is if we can omit copying src to the support dockerfile but it doesn't impact build times much because the src folder is small.

done, as a sense check, i only copied folders not prefixed with a . - these correspond to visible folders on our filesystem.

of the folders seen above, i omitted node_modules (via .dockerignore), support (not used) and docs (no runtime impact).

lmk if it should be otherwise. i've also copied patches over to Dockerfile.support to support dd-trace w/ neverthrow

timotheeg

Looking good!

Is the PR missing a file for prod?

.aws/deploy/support-task-definition.prod.json

support/routes/v2/formsg/index.ts

timotheeg · 2024-04-08T06:06:02Z

.aws/deploy/support-task-definition.staging.json

+        { "name": "OTP_EXPIRY", "valueFrom": "STAGING_OTP_EXPIRY" },
+        { "name": "OTP_SECRET", "valueFrom": "STAGING_OTP_SECRET" },


Should some of these variables removed from the support container? I assume they are not useful there? I don't have a sense of the full list of what's needed and not, but general principle is to only give the app what it needs.

i copied it because i didn't want to sift through and manually find out what env vars are/are not used.

Bit painful yes, but we should do it anyway for good hygiene. If the service doesn't need to know of certain secrets or values, we shouldn't pass them in.

sounds good! you ok if i do this in a follow-up? my immediate priority now is to get functionality up and merged - will address this hygiene issue downstream

timotheeg · 2024-04-08T06:07:30Z

.aws/deploy/support-task-definition.staging.json

+          "name": "DD_SERVICE",
+          "value": "isomer"
+        },
+        {
+          "name": "DD_TAGS",
+          "value": "service:isomer"
+        },


We should use a different service name for this (maybe isomer-support), so we can also see the ECS infra metrics and the endpoint performance under the service

i don't mind this - should we also shift logs?

Yes! Since we are creating a new service, we need to have the telemetry set up and linked well, so we all the drill down capability of DD work.

changed to isomer-support (keep things consistent throughout between files/service)

i've changed it on 3 areas:

support container itself (done via hard-coded env vars instead of via secrets injection through SSM - idt secrets injection adds much here and obfuscates the actual value itself, which is annoying)

updated env vars on dd-agent container too (in support-task-def)

updated dockerLabels on dd-agent

this should allow us to get the tagging separately, do check if i missed out anything!

timotheeg · 2024-04-08T06:08:25Z

.aws/deploy/support-task-definition.staging.json

+      "logConfiguration": {
+        "logDriver": "awslogs",
+        "options": {
+          "awslogs-group": "/aws/elasticbeanstalk/cms-backend-staging-node18/var/log/web.stdout.log",


+1 to creating another log group

Maybe we can drop elasticbeanstalk from the name too...

.aws/deploy/support-task-definition.staging.json

seaerchin · 2024-04-08T06:39:31Z

Looking good!

Is the PR missing a file for prod?
.aws/deploy/support-task-definition.prod.json

@timotheeg yes it is! as mentioned, the CNAME setup + deploy workflow hasn't been done and will be in another PR

seaerchin · 2024-04-09T04:26:17Z

@timotheeg

for the issues listed below, i'll do in another PR

removing extra env vars -> functionality now is intact, want to get this out and fix hygiene issues later (will raise issue)
creating separate log group -> this is because teh creation of a separate log group requires pulumi up + the PR itself to be approved on infra prior to the PR being raised

timotheeg · 2024-04-11T01:33:13Z

.aws/deploy/support-task-definition.staging.json

+        },
+        {
+          "name": "DD_TAGS",
+          "value": "team:isomer"


In case the infra linkage doesn't work (happened in armoury), we might need to set the service here too:

Suggested change

"value": "team:isomer"

"value": "team:isomer,service:isomer-support"

timotheeg · 2024-04-11T01:35:14Z

.github/workflows/deploy_staging.yml

+      cicd-role: "arn:aws:iam::095733531422:role/isomer-infra-github-oidc-role-16ea937"
+      ecr-repository: "isomer-infra-staging-ecr"
+      ecs-cluster-name: "isomer-stg-ecs"
+      ecs-web-service-name: "stg-support-ecs-service"


Naming convention doens't seem to match the deploy-app name? Did you mean

Suggested change

ecs-web-service-name: "stg-support-ecs-service"

ecs-web-service-name: "isomer-support-stg-ecs-service"

…ort` and team is isomer

* fix: off-by-one error for month number (#1294) * ref(app): shift support flows into separate folder + ecs service (#1269) * feat(caddyfile): add initial caddyfile * refactor(infra): shift infra stuff to new folder * chore(formsg): shift stuff into sub folder * refactor(server): shift infra stuff out * refactor(common): shift shared stuff into common folder * build(package/tsconfig): update packages + paths * refactor(server): swap to ts * refactor(request): update type * feat(infra/routes): add new router * refactor(router): shift initialisation in * refactor(middleware): shift shared stuff out * chore(exports): add exports from indexs * chore(featureflag): disable single export * feat(docker): add containers in docker-compose for infra * feat(deploy): update deploy stuff * chore(cleanup): remove unused/extra code in infra/server * fix(aws_deploy): update deploy file * fix(task def): removed portmappings * fix(dockerfile): copy src files over * fix(docker): update port mappings as ecs uses same network * fix(caddy): update to use local dockerfile * fix(caddyfile): upate proxies * fix(caddyfile): try to netweork via loopback * revert(aws_deploy): revert previous changes * revert(deploy_staging): revert * revert(task def): revert be task def * revert(src): revert files that were used for sidecar * refactor(support): shift into new container * fix(docker): update dockerfile + add health check * fix(aws_deploy): update tag * refactor(index): remove unsnued * fix(app spec): do replacement for container name and port * fix(ci): update image names * feat(support): connect db * chore(request): remove unused import * refactor(dockerfile): update to explicitly copy * chore(index): add a line space * refactor(support task def): update so that dd-service is `isomer-support` and team is isomer * chore(formsg tests): shift tests from `src` -> `support` * ci(sidecar): add deploy files for prod (#1285) * feat(deploy_prod): update workflow to include deploy_support * feat(support-task-def): add prod * fix(task def): update dd tags to include service * fix(dockerfile): revert to copy . (#1304) * chore: bump version to v0.79.0 --------- Co-authored-by: Harish <[email protected]> Co-authored-by: Hsu Zhong Jun <[email protected]> Co-authored-by: Timothee Groleau <[email protected]>

seaerchin force-pushed the ref/sidecar branch from 6b944ee to b790ae6 Compare April 4, 2024 03:37

seaerchin changed the title ~~Ref/sidecar~~ ref(app): shift support flows into separate folder + ecs service Apr 4, 2024

seaerchin force-pushed the ref/sidecar branch from 32b46cb to 44c5e20 Compare April 4, 2024 05:44

seaerchin marked this pull request as ready for review April 4, 2024 05:44

seaerchin requested a review from a team April 4, 2024 05:44

seaerchin commented Apr 4, 2024

View reviewed changes

kishore03109 approved these changes Apr 5, 2024

View reviewed changes

timotheeg reviewed Apr 8, 2024

View reviewed changes

seaerchin requested a review from timotheeg April 9, 2024 05:13

seaerchin mentioned this pull request Apr 9, 2024

ci(sidecar): add deploy files for prod #1285

Merged

2 tasks

seaerchin requested a review from a team April 9, 2024 09:30

timotheeg reviewed Apr 11, 2024

View reviewed changes

seaerchin added 16 commits April 15, 2024 19:28

feat(caddyfile): add initial caddyfile

942afd2

refactor(infra): shift infra stuff to new folder

d94acfa

chore(formsg): shift stuff into sub folder

1c8ca5e

refactor(server): shift infra stuff out

4863b95

refactor(common): shift shared stuff into common folder

f87479d

build(package/tsconfig): update packages + paths

34494a3

refactor(server): swap to ts

c8a5ab3

refactor(request): update type

cc3f8e0

feat(infra/routes): add new router

5aed04e

refactor(router): shift initialisation in

5b3a12d

refactor(middleware): shift shared stuff out

92f9b9d

chore(exports): add exports from indexs

a7f3159

chore(featureflag): disable single export

ac5edd7

feat(docker): add containers in docker-compose for infra

857c3d9

feat(deploy): update deploy stuff

c1594da

chore(cleanup): remove unused/extra code in infra/server

ae26953

seaerchin added 19 commits April 15, 2024 19:28

fix(caddy): update to use local dockerfile

7406638

fix(caddyfile): upate proxies

e24e5d3

fix(caddyfile): try to netweork via loopback

d2f5443

revert(aws_deploy): revert previous changes

7cd1f73

revert(deploy_staging): revert

8dbf034

revert(task def): revert be task def

237c24f

revert(src): revert files that were used for sidecar

d274644

refactor(support): shift into new container

b409b19

fix(docker): update dockerfile + add health check

151a33c

fix(aws_deploy): update tag

3d2c030

refactor(index): remove unsnued

8e5c655

fix(app spec): do replacement for container name and port

eefb76e

fix(ci): update image names

3a15a9c

feat(support): connect db

899b810

chore(request): remove unused import

3fe9569

refactor(dockerfile): update to explicitly copy

fdc82c1

chore(index): add a line space

b9fc8c7

refactor(support task def): update so that dd-service is `isomer-supp…

0a9e741

…ort` and team is isomer

chore(formsg tests): shift tests from src -> support

49d3f3b

seaerchin force-pushed the ref/sidecar branch from 5ed4af3 to 49d3f3b Compare April 15, 2024 11:30

seaerchin merged commit b821ef3 into develop Apr 15, 2024
5 checks passed

seaerchin deleted the ref/sidecar branch April 15, 2024 11:37

This was referenced Apr 15, 2024

release v0.79.0 #1302

Closed

release v0.79.0 #1305

Closed

release v0.79.0 #1307

Closed

release v0.79.0 #1309

Merged

This was referenced Jun 27, 2024

release v0.90.0 #1417

Closed

release v0.90.0 #1419

Closed

dcshzj mentioned this pull request Jun 27, 2024

release v0.90.0 #1421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(app): shift support flows into separate folder + ecs service #1269

ref(app): shift support flows into separate folder + ecs service #1269

seaerchin commented Apr 4, 2024 •

edited

Loading

seaerchin Apr 4, 2024

timotheeg Apr 8, 2024

seaerchin Apr 8, 2024

seaerchin Apr 4, 2024

kishore03109 Apr 5, 2024

seaerchin Apr 8, 2024

seaerchin Apr 4, 2024

seaerchin Apr 4, 2024

seaerchin Apr 4, 2024

kishore03109 left a comment

kishore03109 Apr 5, 2024

kishore03109 Apr 5, 2024

seaerchin Apr 8, 2024 •

edited

Loading

kishore03109 Apr 5, 2024

seaerchin Apr 8, 2024

seaerchin Apr 9, 2024

timotheeg left a comment

timotheeg Apr 8, 2024

seaerchin Apr 8, 2024

timotheeg Apr 9, 2024

seaerchin Apr 9, 2024

timotheeg Apr 8, 2024

seaerchin Apr 8, 2024

timotheeg Apr 8, 2024

seaerchin Apr 9, 2024

seaerchin Apr 9, 2024

timotheeg Apr 8, 2024

seaerchin commented Apr 8, 2024 •

edited

Loading

seaerchin commented Apr 9, 2024

timotheeg Apr 11, 2024

timotheeg Apr 11, 2024

		{ "name": "OTP_EXPIRY", "valueFrom": "STAGING_OTP_EXPIRY" },
		{ "name": "OTP_SECRET", "valueFrom": "STAGING_OTP_SECRET" },

	"value": "team:isomer"
	"value": "team:isomer,service:isomer-support"

	ecs-web-service-name: "stg-support-ecs-service"
	ecs-web-service-name: "isomer-support-stg-ecs-service"

ref(app): shift support flows into separate folder + ecs service #1269

ref(app): shift support flows into separate folder + ecs service #1269

Conversation

seaerchin commented Apr 4, 2024 • edited Loading

Problem

Solution

Tests

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

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin Apr 8, 2024 • edited Loading

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

timotheeg left a 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

seaerchin commented Apr 8, 2024 • edited Loading

seaerchin commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin commented Apr 4, 2024 •

edited

Loading

seaerchin Apr 8, 2024 •

edited

Loading

seaerchin commented Apr 8, 2024 •

edited

Loading