health check: adding support for basic mysql health checks#3502
health check: adding support for basic mysql health checks#3502juchem wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
ad39710 to
d9ee136
Compare
|
@juchem thanks for the contribution. This new health checker should be implemented with our new health checker extension system. See the Redis one for example: https://github.com/envoyproxy/envoy/tree/master/source/extensions/health_checkers Can you convert it over to an optional extension in a new mysql directory? Thank you! |
|
sure, working on it |
|
@mattklein123 done |
|
@juchem sorry for the delay here. I will review by the end of the week. |
|
@rshriram I'm going to take a pass on this now. Is anyone from VMWare interested in helping to review? |
|
Tagging @rshriram as I think he will help review also. |
mattklein123
left a comment
There was a problem hiding this comment.
Overall, looks great. Thanks for working on this. Some comments to get started with.
There was a problem hiding this comment.
nit: period end of sentence.
There was a problem hiding this comment.
Can we add a validation that this is not empty?
There was a problem hiding this comment.
I believe an empty string is actually a valid case for an anonymous user auth
source/common/common/byte_order.h
Outdated
There was a problem hiding this comment.
Can you merge master also when you update? I think @zuercher just added these in his Thrift PR.
There was a problem hiding this comment.
Check out the recent thrift PR. Can we add this extension to CODEOWNERS?
There was a problem hiding this comment.
that might have been added by mistake while rebasing
There was a problem hiding this comment.
nvm, I've mixed things up in my head
There was a problem hiding this comment.
is this the PR you're referring to? #3789
I'm confused about what "Can we add this extension to CODEOWNERS?" means
There was a problem hiding this comment.
https://github.com/envoyproxy/envoy/blob/master/CODEOWNERS. See the new extension policy. Each extension needs owners that will handle code reviews.
There was a problem hiding this comment.
:)
kinda saw that coming, but as I transition to a new team I won't have the cycles to spare, unfortunately
There was a problem hiding this comment.
@juchem is there someone on your old team who would want to help maintain this? If not, another option is to just keep it as a private or public out of tree filter for now.
There was a problem hiding this comment.
@austin-zhu @Jason-Jian @ken-experiment any interest in helping maintain the MySQL health check?
There was a problem hiding this comment.
Sign me up. although I'd need to come up to speed first. :)
There was a problem hiding this comment.
There you go. I'll spend some time this week with Jason to provide an overview of the code structure, then I'll add him as a maintainer of the MySQL health checker in the CODEOWNERS file.
There was a problem hiding this comment.
There is some missing coverage for error cases in this file. Can you check the coverage build in CI and add some more tests?
There was a problem hiding this comment.
@mattklein123 where can I find the coverage build in CI? I can't find the build checks. Maybe because the PR was closed?
There was a problem hiding this comment.
nit: can we make all these methods camelCase per normal Envoy style?
There was a problem hiding this comment.
of course. sorry about that, it's just muscle memory
There was a problem hiding this comment.
I don't know the answer to this, but between the Thrift, Mongo, and now this, is there any way to share any of this type of functionality in a common EndianBufferHelper or something? cc @zuercher
There was a problem hiding this comment.
I think if we can agree on parsing directly from the buffer (a la mongo/thrift) vs linearizing the entire request before parsing (mysql), we could come up with a helper that supports all 3 protocols. Thrift requests can be almost arbitrarily large, and depending on configuration the size may not be known at the start of the request, so it's not reasonable to linearize them.
Between the three I already see 8/16/24/32/64-bit integers (some signed, some unsigned) plus both null- and non-terminated strings, so there's a lot to go into this utility already.
There was a problem hiding this comment.
Yeah, +1. @juchem can we at least file a tech debt tracking issue on figuring this out?
There was a problem hiding this comment.
I agree it's better. I'll check Mongo/Thrift implementation to see how they do it and depending on the work I'll either file a tech debt or update the PR.
There was a problem hiding this comment.
Technically, I think you need to handle partial data here (and elsewhere in similar parsing functions)? There is no guarantee that all the data will come in 1 shot. If that is something that you don't think we need to handle, we should clearly document that and possibly add a TODO here, though I would rather we just handle it. Thoughts?
There was a problem hiding this comment.
I haven't seen the need for it just yet, so I believe leaving it as a TODO might suffice. As for how partial data is handled elsewhere in Envoy, does it accumulate partial data in a local buffer and parses in one shot or is there support for a parsing state machine? Implementing such a machine by hand sounds counter-productive and error prone.
|
Hey guys, thanks for all the feedback. I just got out on paternal leave so it'll take me a while until I can work on this again. |
There was a problem hiding this comment.
What does linearize mean here? Deserialize the payload ? If so I would rephrase accordingly. Because a stream of bytes is inherently linear.
There was a problem hiding this comment.
linearize is a Buffer operation. I'll check Mongo's implementation as mentioned somewhere else to see how it reads straight from the buffer
There was a problem hiding this comment.
A whole lot of functions here can be useful for anyone wanting to build a full blown MySQL codec in Envoy. I suggest moving these useful MySQL parsing stuff (login, packet okay and others) into a generic MySQL folder (I don’t know the right destination for it given that there is no MySQL filter), but it would provide the filter implementor a jump start. With all the codec functions abstracted, you could encapsulate checks for partial packets etc into the codec, such that the health check is dramatically simplified into
MySQL.doLogin(user)
MySQL.resp == okay -> health check ok
There was a problem hiding this comment.
sounds like a good idea. I'm not sure that would be useful out-of-the-box for a filter implementor, but it's a starting skeleton as you mentioned. I'll see what I can come up with
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
I think @juchem is on paternity leave. I will just close for now and we can reopen when he is back. |
|
Hey @mattklein123, I'm back from paternity leave. I don't have too many cycles to put on this PR so let's try to find a compromise in case I can't work on all requests. I'll strive to take care of them all, though. |
|
@juchem I reopened it. If you push new commits it should build again. |
There was a problem hiding this comment.
I don't think the big- or little-endian appellation applies to single bytes, so probably undo this one.
But for the rest of the methods, is there a specific reason for renaming them all? For example, are you planning on moving this helper class out of the thrift extension so it can be reused elsewhere?
|
It is a preparation to move it outside of thrift. Renaming the 8 bit
version was mostly due to uniformity, but I agree it might have been a bad
idea.
Btw, I forgot to tag the PR as WIP since I still have more changes to make.
How can I cause the coverage tests to kick in?
…On Wed, Jul 11, 2018, 8:16 PM Stephan Zuercher ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In source/extensions/filters/network/thrift_proxy/buffer_helper.h
<#3502 (comment)>:
> @@ -69,104 +69,104 @@ class BufferHelper {
* @param offset offset into buffer to peek at
* @return the int8_t at offset in buffer
*/
- static int8_t peekI8(Buffer::Instance& buffer, uint64_t offset = 0);
+ static int8_t peekBEI8(Buffer::Instance& buffer, uint64_t offset = 0);
I don't think the big- or little-endian appellation applies to single
bytes, so probably undo this one.
But for the rest of the methods, is there a specific reason for renaming
them all? For example, are you planning on moving this helper class out of
the thrift extension so it can be reused elsewhere?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3502 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABr37ILEAGFSDj1WLjSBoppiIdYi3Tygks5uFr-WgaJpZM4USY2J>
.
|
|
You have to add a commit to get CI to run again, unfortunately. |
|
took care of the coverage, it was a simple issue in the way test packets were built |
|
@juchem so is this ready for re-review? |
|
Yes |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@juchem can you merge master? Then I promise I will look at this PR this week. Thank you! |
b4d23d4 to
2607c33
Compare
|
alright. I'll rebase and make sure tests run, then @Jason-Jian will commandeer the PR |
Adding support for basic mysql health checks, for feature parity with HAProxy. Only passowrdless users are supported, so it's generally a good idea to create a db user with no privileges exclusively for the health checks. Signed-off-by: Marcelo Juchem <juchem@gmail.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
Sorry will look at this soon. |
| <envoy_api_field_core.HealthCheck.unhealthy_edge_interval>`, :ref:`unhealthy to healthy | ||
| <envoy_api_field_core.HealthCheck.healthy_edge_interval>` and for subsequent checks on | ||
| :ref:`unhealthy hosts <envoy_api_field_core.HealthCheck.unhealthy_interval>`. | ||
| * health check: added basic support for :ref:`MySQL health checks <config_health_checkers_mysql>`. |
There was a problem hiding this comment.
Should be moved to the 1.8.0 section.
| * @param Size how many bytes to read out of the buffer. | ||
| * @param Endianness specifies the byte order to use when decoding the integral. | ||
| */ | ||
| template <typename T, ByteOrder Endianness = ByteOrder::Host, size_t Size = sizeof(T)> |
There was a problem hiding this comment.
Nice. I really like this set of changes. Can we please move this change (and the associated Thrift changes) into a separate change? We should make sure the new buffer functions have explicit tests in buffer_impl_test. Let's get that change first and then we can merge master into this change and get this in. Thank you!
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@mattklein123 FYI there were conflicts with upstream when I tried to rebase, I haven't fully resolved them yet. To clarify, your suggestion was to separate this PR into 2, with explicit tests for the buffer functions, correct? I can reopen the PR once I get these resolved. |
|
@Jason-Jian Matt's out for a bit, but I think I understand what he wanted: PR 1: the include/envoy/buffer and source/common/byte_order changes, and the refactoring of thrift_proxybuffer_helper.h/cc to use them. This PR should include direct tests for the byte_order/buffer code (it may be that you can move test cases from buffer_helper_test.cc, it's up to you) PR 2: the mysql health check extension and it's API changes. The idea is that as much as possible, PRs related to extensions should avoid modifying main-line Envoy code. |
#4344) Extract common buffer helper functions. This is split out and rebased with upstream as per discussion in #3502 (comment). Risk Level: Medium Testing: Unit tests added for buffer and byte_order changes Docs Changes: N/A Release Notes: Per discussion with @zuercher this change itself shouldn't need an entry in version_history.rst Signed-off-by: Jason Jian <jason.jian@airbnb.com>
Description:
Adding support for basic mysql health checks, for feature parity with
HAProxy. Only passowrdless users are supported, so it's generally a good
idea to create a db user with no privileges exclusively for the health
checks.
Risk Level: Medium
Testing:
Unit tests added for the following scenarios:
{reuse_connection, dont_reuse_connection} x ({success, timeout, remote_close} + { bad_server_greeting, bad_server_reply} x coverage)
Docs Changes:
Inline documentation added for the new health check API.
Release Notes:
Added an entry to
version_history.rstwith the new feature.cc: @lap1817
Fixes #3501