golang callaback: add DownstreamSslConnection#43480
Conversation
d9c8ed6 to
14987e0
Compare
14987e0 to
1346b00
Compare
1346b00 to
be7e9ce
Compare
wdauchy
left a comment
There was a problem hiding this comment.
nice work on the feature parity with Lua, the Go interface and tests look solid. I have a few concerns mostly around the C API surface area and some consistency nits.
| } | ||
|
|
||
| func (n *connectionCallback) DownstreamSslConnection() api.SslConnection { | ||
| panic("implement me") |
There was a problem hiding this comment.
the interface contract says "returns nil if the connection is not secured with SSL/TLS". panic("implement me") will crash at runtime instead. consider returning nil here to match the documented behavior, same as the other unimplemented stubs that return zero values.
There was a problem hiding this comment.
Returning nil here would actually be misleading, since per the interface contract nil specifically means "the connection is not secured with SSL/TLS". That would be incorrect - the connection might be secured, we just haven't implemented this functionality for network filters yet.
The panic("implement me") pattern is consistent with other unimplemented methods in this file (e.g., GetRouteName, FilterChainName, Protocol, ResponseCode, DynamicMetadata, UpstreamClusterName, etc.). The network filter is less commonly used than the HTTP filter, so these stubs exist to fulfill the Go interface while signaling that the functionality isn't available yet.
If/when we implement SSL connection support for network filters, we'll replace the panic with the actual implementation.
Which could be done in a followup PR if needed
c75025e to
5baf742
Compare
4628e67 to
dd97604
Compare
wdauchy
left a comment
There was a problem hiding this comment.
looks good to me. the refactoring to use EnvoyValue enum dispatch with getStringValue/getIntegerValue/getStringsValue is much cleaner than the original 21 separate C API functions. the null byte separator for SANs and the streamInfo() revert are also addressed. nice work.
|
cc: @doujiang24 if you could re-review the PR |
doujiang24
left a comment
There was a problem hiding this comment.
Sorry for the late, just back from a long vocation.
adding downstreamSslConnection to have 1:1 matching with Lua features Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
adding additional GetStringsValue Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
clone string to avoid dept Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
keeping the String `\0` separated Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
unify test header Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
6e50073 to
fbb0cd7
Compare
|
the CI failure is unrelated to this PR and is failing on other PR |
|
/retest |
|
@botengyao could we also get a review with write access ? |
|
/retest |
adding downstreamSslConnection to have 1:1 matching with Lua features Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features Additional Description: Risk Level: Low Testing: Integration test Docs Changes: No Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com> Signed-off-by: bjmask <11672696+bjmask@users.noreply.github.com>
adding downstreamSslConnection to have 1:1 matching with Lua features Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features Additional Description: Risk Level: Low Testing: Integration test Docs Changes: No Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
adding downstreamSslConnection to have 1:1 matching with Lua features Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features Additional Description: Risk Level: Low Testing: Integration test Docs Changes: No Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
adding downstreamSslConnection to have 1:1 matching with Lua features Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features Additional Description: Risk Level: Low Testing: Integration test Docs Changes: No Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
adding downstreamSslConnection to have 1:1 matching with Lua features Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features Additional Description: Risk Level: Low Testing: Integration test Docs Changes: No Release Notes: Yes Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
adding downstreamSslConnection to have 1:1 matching with Lua features
Commit Message: adding downstreamSslConnection to have 1:1 matching with Lua features
Additional Description:
Risk Level: Low
Testing: Integration test
Docs Changes: No
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]