-
Notifications
You must be signed in to change notification settings - Fork 338
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
fix(meshpassthrough): disable tls and http inspector for mysql protocol #12839
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Lukasz Dziedziak <[email protected]>
Reviewer Checklist🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
|
Signed-off-by: Lukasz Dziedziak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One Q
@@ -52,6 +52,9 @@ func validateDefault(conf Conf) validators.ValidationError { | |||
} | |||
uniqueDomains := map[portProtocol]map[string]bool{} | |||
for i, match := range conf.AppendMatch { | |||
if match.Protocol == MysqlProtocol && match.Port == nil { | |||
verr.AddViolationAt(validators.RootedAt("appendMatch").Index(i).Field("port"), "port must be defined for Mysql protocol") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: usual spelling is MySQL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lowercase each protocol so we should not make an exception for MySQL https://github.com/kumahq/kuma/blob/master/pkg/core/resources/apis/mesh/dataplane_helpers.go#L23.
@@ -370,6 +370,9 @@ resources: | |||
statPrefix: meshpassthrough_http_80 | |||
name: meshpassthrough_http_80 | |||
listenerFilters: | |||
- name: envoy.filters.listener.original_dst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this original dest filter required always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/listener_filters/original_dst_filter#linux. Previously, I overlooked this filter because our TLS and HTTP inspectors were handling everything correctly. However, after disabling the TLS/HTTP filters, matching stopped working, so I realized that the original_dst_filter was missing. I don’t think we need to disable it in other scenarios.
Looks good to me, and I think this needs to be discussed/reviewed by extended members as it introduces a new protocol and touches many files. |
Signed-off-by: Lukasz Dziedziak <[email protected]>
Signed-off-by: Lukasz Dziedziak <[email protected]>
Motivation
When trying to communicate with MySQL, the user wants to enable communication with the database. Unfortunately, the MySQL protocol is slightly different, and the usual method of configuring filter chains does not work when using MeshPassthrough.
Implementation information
original_dst
listener filtermysql
which works only with CIDR/IP and requires port since we need to disabletls_inspector
andhttp_inspector
listener filters for the porttcp_proxy
but with with disabled listener filtersThe user may have rules with HTTP traffic and TCP traffic on the same port matching different IP. example: tls matching on port 8080(IP: 192.168.1.1) and TCP matching on 8080 (IP: 172.1.1.1), that would disable TLS inspector on the port 8080 and wouldn't match
Supporting documentation
https://dev.mysql.com/doc/dev/mysql-server/8.4.3/page_protocol_connection_phase_packets.html
envoyproxy/envoy#21044