-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#175014199] Health checks #90
Changes from 11 commits
0f58421
a2bc002
76f9bd5
832ff21
f5e1c75
acb00cf
dcd19fd
1116597
9292871
a365426
8433210
dff6096
ce4a61c
9f833a7
09211cc
e4c5e67
aefad7c
ba5b0ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { taskEither, fromLeft } from "fp-ts/lib/TaskEither"; | ||
import { HealthCheck } from "../../utils/healthcheck"; | ||
import { InfoHandler } from "../handler"; | ||
|
||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
describe("InfoHandler", () => { | ||
it("should return an internal error if the application is not healthy", async () => { | ||
const healthCheck: HealthCheck = fromLeft(["failure 1", "failure 2"]); | ||
const handler = InfoHandler(healthCheck); | ||
|
||
const response = await handler(); | ||
|
||
expect(response.kind).toBe("IResponseErrorInternal"); | ||
}); | ||
|
||
it("should return a success if the application is healthy", async () => { | ||
const healthCheck: HealthCheck = taskEither.of(true); | ||
const handler = InfoHandler(healthCheck); | ||
|
||
const response = await handler(); | ||
|
||
expect(response.kind).toBe("IResponseSuccessJson"); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,37 @@ | ||
import * as express from "express"; | ||
import { wrapRequestHandler } from "io-functions-commons/dist/src/utils/request_middleware"; | ||
import { | ||
IResponseErrorInternal, | ||
IResponseSuccessJson, | ||
ResponseErrorInternal, | ||
ResponseSuccessJson | ||
} from "italia-ts-commons/lib/responses"; | ||
import * as packageJson from "../package.json"; | ||
import { checkApplicationHealth, HealthCheck } from "../utils/healthcheck"; | ||
|
||
interface IInfo { | ||
version: string; | ||
} | ||
|
||
type InfoHandler = () => Promise<IResponseSuccessJson<IInfo>>; | ||
type InfoHandler = () => Promise< | ||
IResponseSuccessJson<IInfo> | IResponseErrorInternal | ||
>; | ||
|
||
export function InfoHandler(): InfoHandler { | ||
return async () => { | ||
return ResponseSuccessJson({ | ||
version: packageJson.version | ||
}); | ||
}; | ||
export function InfoHandler(healthCheck: HealthCheck): InfoHandler { | ||
return () => | ||
healthCheck | ||
.fold<IResponseSuccessJson<IInfo> | IResponseErrorInternal>( | ||
problems => ResponseErrorInternal(problems.join("\n\n")), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure than any critical information come from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. For the case I tested I saw no problems (resource unavailable, wrong credentials). Still it's important to point out, thanks. Do you see any case which can be problematic? Please also consider that, for what concerns to functions, the endpoint is unauthenticated but not public, it's reachable only from our private network. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could anyway return a generic Error message of what type of error occurs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lgtm as soon as it'll be easy to troubleshoot. @gunzip thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
_ => | ||
ResponseSuccessJson({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that would be useful to return all the statuses even in case of success (to build a status dashboard of all services). we should think about some common vocabulary for status checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something like this: https://tools.ietf.org/html/draft-inadarei-api-health-check-04 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What d'you mean? We are checking only the current application status - at most, if it can reach an external resources. So I think we just have a single pass/fail status
I agree indeed.
I like this approach, but I notice that the above is a draft. Meanwhile, we are using a As for now, we need:
We can model further and define a proper encoding for failure reports, to make them machine-readable. However, I see no use for that so far. |
||
version: packageJson.version | ||
}) | ||
) | ||
.run(); | ||
} | ||
|
||
export function Info(): express.RequestHandler { | ||
const handler = InfoHandler(); | ||
const handler = InfoHandler(checkApplicationHealth()); | ||
|
||
return wrapRequestHandler(handler); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
import { Either } from "fp-ts/lib/Either"; | ||
import { NonEmptyString } from "italia-ts-commons/lib/strings"; | ||
import { MailerConfig } from "../config"; | ||
|
||
const aMailFrom = "example@test.com"; | ||
|
||
const noop = () => {}; | ||
const expectRight = <L, R>(e: Either<L, R>, t: (r: R) => void = noop) => | ||
e.fold( | ||
_ => | ||
fail(`Expecting right, received left. Value: ${JSON.stringify(e.value)}`), | ||
_ => t(_) | ||
); | ||
|
||
const expectLeft = <L, R>(e: Either<L, R>, t: (l: L) => void = noop) => | ||
e.fold( | ||
_ => t(_), | ||
_ => | ||
fail(`Expecting left, received right. Value: ${JSON.stringify(e.value)}`) | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice approach. Can we move these methods to a test utility library to share this logic across all the projects? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to write a jest extension module with custom matchers for io-ts/fp-ts. Maybe one day ;) |
||
|
||
describe("MailerConfig", () => { | ||
it("should decode configuration for sendgrid", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
SENDGRID_API_KEY: "a-sg-key" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.SENDGRID_API_KEY).toBe("a-sg-key"); | ||
expect(typeof value.MAILUP_USERNAME).toBe("undefined"); | ||
}); | ||
}); | ||
|
||
it("should decode configuration for sendgrid", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
SENDGRID_API_KEY: "a-sg-key" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.SENDGRID_API_KEY).toBe("a-sg-key"); | ||
expect(typeof value.MAILUP_USERNAME).toBe("undefined"); | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a duplicate test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do yo mean the whole case or just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole test is exactly the same as the previous one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, didn't notice that. Good catch! |
||
|
||
it("should decode configuration for sendgrid even if mailup conf is passed", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
SENDGRID_API_KEY: "a-sg-key", | ||
MAILUP_USERNAME: "a-mu-username", | ||
MAILUP_SECRET: "a-mu-secret" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.SENDGRID_API_KEY).toBe("a-sg-key"); | ||
}); | ||
}); | ||
|
||
it("should decode configuration for mailup", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
MAILUP_USERNAME: "a-mu-username", | ||
MAILUP_SECRET: "a-mu-secret" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.MAILUP_USERNAME).toBe("a-mu-username"); | ||
expect(value.MAILUP_SECRET).toBe("a-mu-secret"); | ||
// check types | ||
const _: NonEmptyString = value.MAILUP_SECRET; | ||
const __: NonEmptyString = value.MAILUP_USERNAME; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually a canary to catch possible problems when/if we'll edit the config model. I found that several times the programmatic check doesn't ensure the correct type inference (hence, you have green tests but the project will fail to build). Not exhaustive, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't understand. At runtime we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a type check inside the tests is better to create an explicit type
And check the response. We can use these types even inside the logic with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the check is all about "let me se if the interface I designed actually results in the correct type once I exit the io-ts world". |
||
}); | ||
}); | ||
|
||
it("should decode configuration with multi transport", () => { | ||
const aTransport = { | ||
password: "abc".repeat(5), | ||
transport: "transport-name", | ||
username: "t-username" | ||
}; | ||
const aRawTrasport = [ | ||
aTransport.transport, | ||
aTransport.username, | ||
aTransport.password | ||
].join(":"); | ||
|
||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
MAIL_TRANSPORTS: [aRawTrasport, aRawTrasport].join(";") | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.MAIL_TRANSPORTS).toEqual([aTransport, aTransport]); | ||
}); | ||
}); | ||
|
||
it("should decode configuration for mailhog", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "dev", | ||
MAILHOG_HOSTNAME: "a-mh-host" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectRight(result, value => { | ||
expect(value.MAILHOG_HOSTNAME).toBe("a-mh-host"); | ||
}); | ||
}); | ||
|
||
it("should require mailhog if not in prod", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "dev" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectLeft(result); | ||
}); | ||
|
||
it("should require at least on transporter if in prod", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectLeft(result); | ||
}); | ||
|
||
it("should not allow mailhog if in prod", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
MAILHOG_HOSTNAME: "a-mh-host" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectLeft(result); | ||
}); | ||
|
||
it("should not decode configuration with empty transport", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production", | ||
MAIL_TRANSPORTS: "" | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectLeft(result); | ||
}); | ||
|
||
it("should not decode configuration when no transporter is specified", () => { | ||
const rawConf = { | ||
MAIL_FROM: aMailFrom | ||
}; | ||
const result = MailerConfig.decode(rawConf); | ||
|
||
expectLeft(result); | ||
}); | ||
|
||
it("should not decode ambiguos configuration", () => { | ||
const withMailUp = { | ||
MAILUP_USERNAME: "a-mu-username", | ||
MAILUP_SECRET: "a-mu-secret" | ||
}; | ||
const withSendGrid = { | ||
SENDGRID_API_KEY: "a-sg-key" | ||
}; | ||
const withMultiTransport = { | ||
MAIL_TRANSPORTS: "a-trasnport-name" | ||
}; | ||
const base = { | ||
MAIL_FROM: aMailFrom, | ||
NODE_ENV: "production" | ||
}; | ||
|
||
const examples = [ | ||
// the following configuration is not ambiguos as sendgrid would override mailup anyway | ||
// see here for the rationale: https://github.com/pagopa/io-functions-admin/pull/89#commitcomment-42917672 | ||
// { ...base, ...withMailUp, ...withSendGrid }, | ||
{ ...base, ...withMultiTransport, ...withSendGrid }, | ||
{ ...base, ...withMailUp, ...withMultiTransport }, | ||
{ ...base, ...withMailUp, ...withSendGrid, ...withMultiTransport } | ||
]; | ||
|
||
examples.map(MailerConfig.decode).forEach(_ => expectLeft(_)); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a type error in here, the test fails.