Skip to content

Add grpc checks support for esm.#319

Open
DaryaShirokova wants to merge 2 commits intohashicorp:mainfrom
bloomberg:upstream
Open

Add grpc checks support for esm.#319
DaryaShirokova wants to merge 2 commits intohashicorp:mainfrom
bloomberg:upstream

Conversation

@DaryaShirokova
Copy link
Copy Markdown

@DaryaShirokova DaryaShirokova commented Jul 30, 2025

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

Adding grpc health check, which is already supported in consul, to consul-esm.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

Can be reverted.

  • If applicable, I've documented the impact of any changes to security controls.

NA.


Add gRPC health check support to Consul ESM:

  • Implement gRPC health check type alongside existing HTTP and TCP checks
  • Refactor check-stopping logic: extract shared stopExistingChecks method from duplicated inline code in updateCheckHTTP and updateCheckTCP
  • Add tlsConfigsEqual helper for TLS config comparison
  • Add gRPC test server utilities and tests for healthy, unhealthy, and TLS scenarios

@hashicorp-cla-app
Copy link
Copy Markdown

hashicorp-cla-app bot commented Jul 30, 2025

CLA assistant check
All committers have signed the CLA.

@hashicorp-cla-app
Copy link
Copy Markdown

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@markcampv markcampv assigned markcampv and unassigned markcampv Jul 30, 2025
@DaryaShirokova DaryaShirokova force-pushed the upstream branch 4 times, most recently from 1973cec to 936881a Compare August 1, 2025 15:19
@DaryaShirokova DaryaShirokova marked this pull request as ready for review August 26, 2025 09:47
@DaryaShirokova DaryaShirokova requested a review from a team as a code owner August 26, 2025 09:47
@drofloh
Copy link
Copy Markdown

drofloh commented Mar 9, 2026

Am interested in this, any plans for it to be merged / GRPC check to be supported in ESM?

@DaryaShirokova
Copy link
Copy Markdown
Author

Am interested in this, any plans for it to be merged / GRPC check to be supported in ESM?

This is still pending review. I see there is a conflict in go.mod now which I will address.

Comment thread check_test.go Outdated
if err != nil {
t.Fatal(err)
}
defer s.Stop()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

defer s.Stop() executes when setUp := func(t *testing.T) *fixture returns, so the Consul test server is gone before the subtest uses runner: &CheckRunner. That makes these cases validate behavior against a dead server rather than stopExistingChecks. Please switch this to t.Cleanup(s.Stop) or keep the server on the fixture and stop it at subtest cleanup time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Should be resolved now, thanks for the comment!

@santoshpulluri
Copy link
Copy Markdown
Contributor

Please update the PR description with appropriate details.
README files needs to be updated to mention the gRPC support with this change.

The changes look good except for the minor comment related to test case.

@DaryaShirokova
Copy link
Copy Markdown
Author

Please update the PR description with appropriate details. README files needs to be updated to mention the gRPC support with this change.

The changes look good except for the minor comment related to test case.

Thanks @santoshpulluri , I have addressed the feedback, could you please have another look?

Copy link
Copy Markdown
Contributor

@santoshpulluri santoshpulluri left a comment

Choose a reason for hiding this comment

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

I reviewed the latest updates. The earlier test lifecycle issue has been addressed, and the README now includes a gRPC check example. The gRPC support is integrated cleanly into the existing check runner flow, reuses Consul’s native gRPC health check implementation, and I don’t see any remaining blocking concerns. Approved.

@DaryaShirokova
Copy link
Copy Markdown
Author

@santoshpulluri thanks for the review! are there any action items on my side before it could be merged or you'll merge it on your side?

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