Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add health check support for Keycloak container #7122

Merged
merged 9 commits into from
Jan 19, 2025
Merged

Add health check support for Keycloak container #7122

merged 9 commits into from
Jan 19, 2025

Conversation

paulomorgado
Copy link
Contributor

@paulomorgado paulomorgado commented Jan 15, 2025 •
edited
Loading

Description

This PR introduces new constants for health checks, including an environment variable (KC_HEALTH_ENABLED), a health check container port (9000), and a health endpoint name (health). The AddKeycloak method is updated to incorporate a health check endpoint and set the health check environment variable to "true" for the Keycloak resource.

Fixes #6113

/cc @davidfowl @DamianEdwards

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@paulomorgado
Add health check support for Keycloak container
b4c775e 
This commit introduces new constants for health checks, including
an environment variable (`KC_HEALTH_ENABLED`), a health check
container port (9000), and a health endpoint name (`health`).
The `AddKeycloak` method is updated to incorporate a health
check endpoint and set the health check environment variable
to "true" for the Keycloak resource.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 15, 2025
@paulomorgado paulomorgado mentioned this pull request Jan 15, 2025
Closed
1 task
@paulomorgado
Refactor Keycloak tests for endpoint annotations and manifest
cd15831 
Updated `AddKeycloakWithDefaultsAddsAnnotationMetadata` to filter `EndpointAnnotation` by name for "http" and "health", adding assertions for the "health" endpoint. Modified `VerifyManifest` to use a specific image reference and added the `KC_HEALTH_ENABLED` environment variable, along with updating the `bindings` section to include the "health" binding.
@davidfowl
Copy link
Member

@DamianEdwards can you review?

DamianEdwards
DamianEdwards requested changes Jan 17, 2025
View reviewed changes
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs Outdated
@@ -59,10 +62,13 @@ public static IResourceBuilder<KeycloakResource> AddKeycloak(
.WithImageRegistry(KeycloakContainerImageTags.Registry)
.WithImageTag(KeycloakContainerImageTags.Tag)
.WithHttpEndpoint(port: port, targetPort: DefaultContainerPort)
.WithHttpEndpoint(targetPort: HealthCheckContainerPort, name: HealthEndpointName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding the management interface port as an optional parameter too (like port) so it can be easily changed by the caller. If it's set then we'd need to set the KC_HTTP_MANAGEMENT_PORT environment variable too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that mean that we should also support KC_HTTP_PORT?

But maybe we could add a managementPort parameter with the same semantics of the port parameter, without messing with target ports.

Would it be feasible to determine the target ports based on the existence or not of KC_HTTP_PORT and KC_HTTP_MANAGEMENT_PORT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe we could add a managementPort parameter with the same semantics of the port parameter, without messing with target ports.

Yeah this is what I think I meant 😄 Just let the user specify a different management port which we'll assign to the proxy port. Target port will stay the same always (unless user manually edits the added the endpoint, which of course they can still do).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added with tests!

@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 17, 2025
@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 17, 2025
@paulomorgado
Refactor Keycloak resource constants and tests
c874176 
- Retained `HealthCheckEnvVarName` constant; changed type of `ManagementInterfaceContainerPort` to `int` for clarity.
- Introduced `defaultEndpointName` and `managementEndpointName` variables in tests to replace hardcoded strings for improved readability.
- Updated JSON structure in `VerifyManifest` to change key from "health" to "management" to align with new naming conventions.
@paulomorgado paulomorgado force-pushed the Keycloak-HealthChecks branch from 1aea6ec to c874176 Compare January 17, 2025 08:39
@davidfowl davidfowl merged commit 21f273f into dotnet:main Jan 19, 2025
9 checks passed
@paulomorgado paulomorgado deleted the Keycloak-HealthChecks branch January 20, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@DamianEdwards DamianEdwards DamianEdwards approved these changes

Assignees

@DamianEdwards DamianEdwards

Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Keycloak - Integrate Health Checks
3 participants
@paulomorgado @davidfowl @DamianEdwards