-
Notifications
You must be signed in to change notification settings - Fork 2
Smoke test setup and initial health check feature #72
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
base: main
Are you sure you want to change the base?
Conversation
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.
We're on our way! 🚀
I've left some thoughts about some possible changes, some are nice-to-haves (e.g. reading env from environment, using uv, using more fixtures) while others might be critical (e.g. changes made to how we build our docker image).
Before merging, I think we should integrate this within our deploy pipeline, likely as a smoketest job that would trigger after a successful deployment:
veda-keycloak/.github/workflows/deploy.yaml
Lines 27 to 28 in ad51835
| jobs: | |
| deploy: |
I don't believe that our current deployment setup allows traffic to port 9000 (e.g. https://auth.openveda.cloud:9000/health/live), meaning that these tests won't pass today. I bumped into this a bit when originally configuring the service, instead using the admin endpoint as a healthcheck:
veda-keycloak/cdk/lib/KeycloakService.ts
Lines 131 to 135 in ad51835
| // TODO: Configure health check on port 9000 with the path '/health' | |
| this.albService.targetGroup.configureHealthCheck({ | |
| path: "/admin/master/console/", | |
| healthyThresholdCount: 3, | |
| }); |
ChatGPT suggestions on how to surface multiple ports via ECS
[!WARNING]
This is untested code.
// 3) Add the second port mapping (e.g., 9000) to the container
const containerDef = this.albService.taskDefinition
.findContainer("keycloak")!; // or .defaultContainer if you only have one
containerDef.addPortMappings({
containerPort: 9000,
});
// 4) Add a second listener on port 9000
const managementListener = loadBalancer.addListener("ManagementListener", {
port: 9000,
protocol: elbv2.ApplicationProtocol.HTTP,
open: true, // open to the world
});
// 5) Attach targets for the container's port 9000
managementListener.addTargets("ManagementTargets", {
protocol: elbv2.ApplicationProtocol.HTTP,
port: 9000,
targets: [
this.albService.service.loadBalancerTarget({
containerName: "keycloak",
containerPort: 9000,
}),
],
healthCheck: {
path: "/health",
},
});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.
Hey @lahirujayathilake, thanks for the updates, great progress! I'm still not sure how these tests would pass, we're testing against http://localhost:8080 but I'm not seeing how that port is exposed in the deployed instances.
|
@alukach, I didn't work on the deployment to expose a management endpoint. However, what we need to do is set the management endpoint as an environment variable so that it gets picked up instead of using the default http://localhost:8080." |
|
Please review my earlier comment. The health checks that are being tested run on port 9000, as per the docs. Neither the deployed instances (important) nor docker compose (less important) expose that port. As such, I don't think these tests would pass today. Prior to merging this PR, I believe we need to address that issue. |
This PR fixes #58 and #44