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.

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

Added support for probes on resources. #6379

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GMouaad
Copy link
Contributor

@GMouaad GMouaad commented Oct 18, 2024

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (890)

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?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 18, 2024
@davidfowl
Copy link
Member

@GMouaad Are you going to go a step further and use WithHttp(s) health check on resources with probes?

@mitchdenny
Copy link
Member

@davidfowl @maddymontaquila @DamianEdwards I think that this is something we should consider for 9.x. Overall I think this approach is sound. One open question is whether we want to automatically inject probes for .NET projects.

We'll also need logic to translate these probes when PublishAsAzureContainerApp is used.

@davidfowl
Copy link
Member

We'll also need logic to translate these probes when PublishAsAzureContainerApp is used.

Yep! Now we can actually do that 😄

@DamianEdwards
Copy link
Member

One open question is whether we want to automatically inject probes for .NET projects.

There's a few challenges with this at the moment:

  1. The health endpoints for projects are defined in the service defaults project and are completely customizable. Ideally we'd have a way for the definitions be shared and thus have a single authority.
  2. The default health endpoints added by the service defaults template aren't secured or isolated today, and thus are only added in the development environment. To add them by default we'd need to revisit this, e.g. isolate them such that they're not exposed on external networks by using host filtering, etc.

@GMouaad
Copy link
Contributor Author

GMouaad commented Oct 23, 2024

@davidfowl from what I understood, moving forward, we want to add Http health checks under the hood, i.e. calling WithHttpHealthCheck(..) inside WithProbe(..), and let the AppHost deal with the probing during development and the whatever infrastructure (ACA,AKS,..) in prod. is that correct?

If that's what you meant, I find it difficult to reason about which scheme to use per default with the current API shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants