-
Notifications
You must be signed in to change notification settings - Fork 328
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
feat(kuma-dp): rework on the virtual probes to support probing tcp and grpc ports #10624
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
…nager Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
… configuration Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
@@ -310,33 +310,5 @@ message Dataplane { | |||
// defined at a Mesh level. | |||
MetricsBackend metrics = 2; | |||
|
|||
message Probes { |
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.
I don't think we can remove it like this. What about universal?
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.
Checked the code, this field only serves Kubernetes scenarios. For universal, there is another field called serviceProbe
. Docs at here
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.
I'm pretty sure that's not going to be fine from a backward compat pov
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.
Hi @lahabana I don't quite understand why removing this field will break backward compatibility?
Without this field, we can still parse the existing dataplane objects stored in the DB when people upgrade their versions. So I think this is not a break change.
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.
There're might be users who rely on this functionality on Universal. Backward compatibility means not only being able to parse the format, but also preserve the functionality. Usually we mark the field as deprecated and remove it after the 2 minor releases
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.
Thanks for the explanation of the "Backward compatibility", I agree with your view of "not only being able to parse the format, but also preserve the functionality". I meant that you guys may be confused this field with the other one serviceProbe
which serves Universal scenarios. The field "probes" here was only serving Kubernetes scenarios. @lobkovilya
Signed-off-by: Jay Chen <[email protected]>
Signed-off-by: Jay Chen <[email protected]>
Hi reviewers, sorry that this is a large PR. To help you review the code easily, let me explain what are included in this PR. Here are the break down tasks:
|
The PR also includes some minor/revelant refactorings. Here are some of them:
|
// /grpc/<port> | ||
// /<port>/<original-path-query> | ||
|
||
if strings.HasPrefix(req.URL.Path, pathPrefixTCP) { |
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.
Please use http.NewServeMux()
like we do for example here:
kuma/pkg/diagnostics/server.go
Lines 56 to 60 in 604610b
mux.HandleFunc("/debug/pprof/", pprof.Index) | |
mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | |
mux.HandleFunc("/debug/pprof/profile", pprof.Profile) | |
mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | |
mux.HandleFunc("/debug/pprof/trace", pprof.Trace) |
instead of parsing the
Path
.
@jijiechen please also think about the case when old kuma-dp connects to the new kuma-cp, I believe it won't work because you removed |
Checklist prior to review
Implements this MADR: gRPC and TCP probes
syscall.Mkfifo
have equivalent implementation on the other OS --ci/
labels to run additional/fewer testsUPGRADE.md
? --