wasm: accept cluster name in grpcCall()#15208
Conversation
Signed-off-by: Shikugawa <rei@tetrate.io>
|
cc @mathetake |
PiotrSikora
left a comment
There was a problem hiding this comment.
As mentioned in #15170, I think this requires patch-level ABI bump, otherwise plugins built with SDKs that support this new format will be loaded sucessfully in Envoy that doesn't support the fallback (i.e. v1.17 or Istio 1.9), and will fail at runtime.
|
@PiotrSikora I think that this is not so bad (even if this is just a workaround) until ABI v1.0.0 released. (It can be also tech debt since we should introduce runtime guard flag of this bahavior) |
PiotrSikora
left a comment
There was a problem hiding this comment.
@PiotrSikora I think that this is not so bad (even if this is just a workaround) until ABI v1.0.0 released. (It can be also tech debt since we should introduce runtime guard flag of this bahavior)
Do you have any thoughts?
There is no perfect answer here.
-
If we don't bump the ABI, then plugins that want to use this feature will load successfully, but they will fail at runtime with
ParseFailure, which is a bit unfortunate. -
If we bump the ABI, then we need to bump the ABI in SDKs in order to use this feature, and then all plugins built using those SDKs will stop loading in older Envoy releases, even when not using this feature, which is definitely a deal breaker.
I'll try to figure out a way to detect optional features in the future, but in the meantime, I think we need to get this in without the ABI bump. Sorry for the noise!
PiotrSikora
left a comment
There was a problem hiding this comment.
Thanks! Looks mostly good, but I'd like to see some changes in tests.
test/extensions/filters/http/wasm/test_data/test_grpc_call_cpp.cc
Outdated
Show resolved
Hide resolved
|
@mandarjog @bianpengyuan @kyessenov we should figure out how/if we can migrate from Google gRPC proto definitions with STS credentails embedded in Istio Stackdriver plugin to using a cluster configured by Istiod. |
|
@PiotrSikora makes sense if possible. @bianpengyuan can you investigate? |
|
Istio would need to offer STS as a feature I think. It's not exposed in the API, so it would require an EnvoyFilter. |
Signed-off-by: Shikugawa <rei@tetrate.io>
|
@PiotrSikora How is going? |
PiotrSikora
left a comment
There was a problem hiding this comment.
LGTM for code, but please convert tests to verify that errors occured (I've asked about this previously, but it looks that you fixed it only in 1 of 6 calls).
test/extensions/filters/http/wasm/test_data/test_grpc_call_cpp.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/wasm/test_data/test_grpc_stream_cpp.cc
Outdated
Show resolved
Hide resolved
|
@Shikugawa friendly ping to get this in before next release. Also, it looks like it needs a merge with |
Signed-off-by: Shikugawa <rei@tetrate.io>
PiotrSikora
left a comment
There was a problem hiding this comment.
Thanks! LGTM, 2 minor nits in comments.
test/extensions/filters/http/wasm/test_data/test_grpc_stream_cpp.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/wasm/test_data/test_grpc_stream_cpp.cc
Outdated
Show resolved
Hide resolved
mattklein123
left a comment
There was a problem hiding this comment.
Small comment, thanks.
/wait
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa rei@tetrate.io
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: Close #15170. Add capability to accept yaml style GrpcService message for grprCall(). It is required to support gRPC related operation on SDK, such as Rust which doesn't have gRPC support. It is required to ease use of grpcCall()
Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]