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

When configured, use the health check of the proxy #1841

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Jan 18, 2023

Changes proposed in this PR:

  • When a service is configured with the correct annotation, a readiness endpoint with be configured in Consul dataplane, and the readiness probe of the sidecar will be configured to use that endpoint to determine the health of the system. Additionally, when t-proxy is enabled, that port shall be in the ExcludeList for inbound connections.

How I've tested this PR:

  • By deploying zookeeper with and without this annotation and noticing the absence of the lack of the following exception.
EndOfStreamException: Unable to read additional data from client sessionid 0x0, likely client has closed socket
	at org.apache.zookeeper.server.NIOServerCnxn.doIO(NIOServerCnxn.java:239)
	at org.apache.zookeeper.server.NIOServerCnxnFactory.run(NIOServerCnxnFactory.java:203)
	at java.lang.Thread.run(Thread.java:748)

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@thisisnotashwin thisisnotashwin requested review from ishustava, a team and jmurret and removed request for a team January 18, 2023 20:30
@thisisnotashwin thisisnotashwin requested review from curtbushko and removed request for jmurret January 18, 2023 20:31
- When a service is configured with the correct annotation, a readiness
  endpoint with be configured in Consul dataplane and the readiness
probe of the sidecar will be configured to use that endpoint to
determine the health of the system. Additionally, when t-proxy is
enabled, that port shall be in the ExcludeList for inbound connections.
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few comments, but don't think they are blocking.

CHANGELOG.md Outdated Show resolved Hide resolved
- Improve changelog wording.
- Make useProxyHealthCheck check for all truthy values.
Copy link
Contributor

@curtbushko curtbushko left a comment

Choose a reason for hiding this comment

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

Looks good! I thought I saw something with CNI but you had that covered and our cleanup in 1.0 had unified a bunch of things that makes is so much cleaner (specifically iptablesConfigJSON being used everywhere).

@thisisnotashwin thisisnotashwin merged commit 1821f08 into main Jan 19, 2023
@thisisnotashwin thisisnotashwin deleted the ashwin/use-proxy-health-check branch January 19, 2023 16:21
david-yu pushed a commit that referenced this pull request Feb 14, 2023
* When a service is configured with the correct annotation, a readiness endpoint with be configured in Consul dataplane and the readiness probe of the sidecar will be configured to use that endpoint to determine the health of the system. Additionally, when t-proxy is enabled, that port shall be in the ExcludeList for inbound connections.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect theme/health-checks About Consul health checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants