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

Update docs with latest guide on health checks #377

Merged
merged 3 commits into from
Sep 2, 2023

Conversation

solmonk
Copy link
Contributor

@solmonk solmonk commented Aug 31, 2023

What type of PR is this?

documentation

Which issue does this PR fix:

What does this PR do / Why do we need it:

  • Updates TargetGroupPolicy reference to mention restrictions on gRPC health checks.
  • Also restructured doc a bit, now includes environment setting in the website

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

Automation added to e2e:

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades. Has updating a running cluster been tested?:

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@coveralls
Copy link

coveralls commented Aug 31, 2023

Pull Request Test Coverage Report for Build 6043366462

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 39.229%

Totals Coverage Status
Change from base Build 6040753957: 0.0%
Covered Lines: 4012
Relevant Lines: 10227

💛 - Coveralls

* Omitting `healthCheck` results in [VPC Lattice default behavior](https://docs.aws.amazon.com/vpc-lattice/latest/ug/target-group-health-checks.html) depending on the protocol.
* Health check is enabled by default for HTTP1 target groups.
* Health check is disabled by default for HTTP2/gRPC target groups.
* For targets behind GRPCRoute, you should create a separate endpoint to enable health checks - HTTP/2 health check directly on gRPC endpoints is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

What this mean? you should create a separate endpoint to enable health checks, other part LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slightly changed the wording there. It means you cannot directly perform HC on the target, so you need to enable another endpoint that is dedicated for health check.

@@ -1,4 +1,4 @@
### Configuration Variables
### Environment Variables
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add please CLUSTER_NAME, since we start using it for tagging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* Omitting `healthCheck` results in [VPC Lattice default behavior](https://docs.aws.amazon.com/vpc-lattice/latest/ug/target-group-health-checks.html) depending on the protocol.
* Health check is enabled by default for HTTP1 target groups.
* Health check is disabled by default for HTTP2/gRPC target groups.
* For targets behind GRPCRoute, you should create a separate endpoint dedicated for health checks - HTTP/2 health check directly on gRPC endpoints is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

you should create a separate endpoint dedicated for health checks

may be just copy aws docs?

Health checks do not support gRPC target group protocol versions. However, if you enable health checks, you must specify the health check protocol version as HTTP1 or HTTP2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to address ambiguity from aws docs there, there are some cases users trying to put HTTP2 health checks on GRPC targets directly and we want to avoid it.

Copy link
Contributor

@zijun726911 zijun726911 left a comment

Choose a reason for hiding this comment

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

LGTM

@mikhail-aws mikhail-aws merged commit 955c64d into aws:main Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants