add http2 ping health checks

[tarat44]

This pull request adds http2 ping health checks and would close #6260. Some tests are failing locally, but the errors seem unrelated.

[hashicorp-cla]

All committers have signed the CLA.

[tarat44]

It looks like there is a bug with how the proxy is set (I have it using the same inject address function as http, but it shouldn't have https/http or slashes in the schema). I will fix this issue tonight.

[tarat44]

It looks like there is a bug with how the proxy is set (I have it using the same inject address function as http, but it shouldn't have https/http or slashes in the schema). I will fix this issue tonight.

It actually appears that proxies do not support ping https://tools.ietf.org/html/rfc7540#section-8.3 (please correct me if my understanding is incorrect) so I will just remove all proxy references completely

[tarat44]

I just realized that the contributing check lists cover this change so I have made many of the missing changes and am pasting it below. I plan to update the documentation and give this list another look this weekend. I apologize for the unintended incompleteness of this pull request.

• Add the field to the Config struct (or an appropriate sub-struct) in
  agent/config/config.go.
• Add the field to the actual RuntimeConfig struct in
  agent/config/runtime.go.
• Add an appropriate parser/setter in agent/config/builder.go to
  translate.
• Add the new field with a random value to both the JSON and HCL blobs in
  TestFullConfig in agent/config/runtime_test.go, it should fail now, then
  add the same random value to the expected struct in that test so it passes
  again.
• Add the new field and it's default value to TestSanitize in the same
  file. (Running the test first gives you a nice diff which can save working
  out where etc.)
• If your new config field needed some validation as it's only valid in
  some cases or with some values (often true).
  • Add validation to Validate in agent/config/builder.go.
  • Add a test case to the table test TestConfigFlagsAndEdgeCases in
    agent/config/runtime_test.go.
• If your new config field needs a non-zero-value default.
  • Add that to DefaultSource in agent/config/defaults.go.
  • Add a test case to the table test TestConfigFlagsAndEdgeCases in
    agent/config/runtime_test.go.
• If your config should take effect on a reload/HUP.
  • Add necessary code to to trigger a safe (locked or atomic) update to
    any state the feature needs changing. This needs to be added to one or
    more of the following places:
    • ReloadConfig in agent/agent.go if it needs to affect the local
      client state or another client agent component.
    • ReloadConfig in agent/consul/client.go if it needs to affect
      state for client agent's RPC client.
  • Add a test to agent/agent_test.go similar to others with prefix
    TestAgent_reloadConfig*.
• Add documentation to website/source/docs/agent/options.html.md.

[tarat44]

I have added documentation and another test. I think this pull request should be ready for review now

[dnephin]

Thank you for the PR, this looks really good!

Sorry it took us so long to respond. Unfortunately there are a number of conflicts now that need to be resolved.

I think once this is rebased to resolve the conflicts, and the additional test is added, this should be ready to merge.

[tarat44]

Thank you for the PR, this looks really good!

Sorry it took us so long to respond. Unfortunately there are a number of conflicts now that need to be resolved.

I think once this is rebased to resolve the conflicts, and the additional test is added, this should be ready to merge.

Thank you for the review, I added the test for cert failure and rebased but still need to work on getting all of the tests to pass and catching up on syncing this new feature with more recent changes to the codebase (ex: proto buffers). I hope to have time to complete the rest of this work sometime this week

[tarat44]

@dnephin this pull request should be good to be looked over again. I hope it is okay that I added H2PING to proto/pbservice/healthcheck.gen.go manually as I was unsure how to generate it withmog as referenced in the comments. If you let me know how I can find and use this package, I would be happy to go back and do it correctly.

[dnephin]

Thank you for implementing this change! Looks great!

I made a couple very minor suggestions. I'm going to apply those changes, and if the tests are still happy I think this is ready to merge.

I noticed that some of the checks use a WaitGroup to wait for the goroutine to stop. An example of adding that is 1910e2a. It's not strictly necessary, but can help prevent flaky tests and makes it easier to detect goroutine leaks in tests if we wait on ones we expect to shutdown. That might be a good addition to this check, as a follow up, if you are interested.

I also noticed the HTTP check supports a proxy. Would a proxy be appropriate for H2 checks as well? Not something we need to add right away, but maybe we could track adding that in a github issue in case it is useful for someone.

[dnephin]

I hope it is okay that I added H2PING to proto/pbservice/healthcheck.gen.go manually as I was unsure how to generate it with mog as referenced in the comments.

Yes, that's perfect, thank you for making the change!

[hashicorp-ci]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/348014.

[tarat44]

Thank you for implementing this change! Looks great!

I made a couple very minor suggestions. I'm going to apply those changes, and if the tests are still happy I think this is ready to merge.

I noticed that some of the checks use a WaitGroup to wait for the goroutine to stop. An example of adding that is 1910e2a. It's not strictly necessary, but can help prevent flaky tests and makes it easier to detect goroutine leaks in tests if we wait on ones we expect to shutdown. That might be a good addition to this check, as a follow up, if you are interested.

I also noticed the HTTP check supports a proxy. Would a proxy be appropriate for H2 checks as well? Not something we need to add right away, but maybe we could track adding that in a github issue in case it is useful for someone.

Hi @dnephin, thank you for the approval and suggestions. After briefly reviewing the CONNECT method on the http2 RFC, it seems like it might not support ping frames, but I have not had direct experience with proxies over http2 so I could be mistaken and will look into this further. I created a PR for WaitGroups and also added graceful shutdown of the client connection (which I only just noticed was not implemented- apologies for missing it earlier) #9995. I hope that it is okay that I combined both the WaitGroup feature and the quick fix to gracefully close connections in the same PR- I am happy to separate them if necessary.