Skip to content

sip_proxy: SIP protocol support in envoy#18039

Merged
mattklein123 merged 21 commits intoenvoyproxy:mainfrom
durd07:sip-proxy
Sep 28, 2021
Merged

sip_proxy: SIP protocol support in envoy#18039
mattklein123 merged 21 commits intoenvoyproxy:mainfrom
durd07:sip-proxy

Conversation

@dorisd0102
Copy link
Copy Markdown
Contributor

@dorisd0102 dorisd0102 commented Sep 9, 2021

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
-->

Commit Message: SIP protocol support in envoy
[Issues]: #16247
Additional Description:
Risk Level: Medium
Testing: unit test & manual testing
Docs Changes: add sip proxy docs
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@durd07 @nearbyfly @dorisd0102

Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@repokitteh-read-only
Copy link
Copy Markdown

Hi @dorisd0102, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18039 was opened by dorisd0102.

see: more, trace.

@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Sep 9, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18039 was opened by dorisd0102.

see: more, trace.

Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@ggreenway
Copy link
Copy Markdown
Member

Regarding the docs error, this should be resolved soon. See #17826 (comment).

@dorisd0102
Copy link
Copy Markdown
Contributor Author

@mattklein123
This PR is a contrib for sip proxy, could you help to review? Thanks.

@mattklein123
Copy link
Copy Markdown
Member

Please remove WIP from the title, merge main, make it pass CI, etc. and then I will take a look.

/wait

Signed-off-by: Mingling <mingling.ding01@gmail.com>
@dorisd0102 dorisd0102 changed the title [WIP] sip_proxy: SIP protocol support in envoy sip_proxy: SIP protocol support in envoy Sep 17, 2021
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@dorisd0102
Copy link
Copy Markdown
Contributor Author

Hi @ggreenway,

the doc build failed in this PR.
I refer to the #17826 and do some changes.
But still reports following warning, could you help to check? Many thanks.

/tmp/tmpav26zfxu/generated/rst/api-v3/config/listener/v3/listener_components.proto.rst:108: WARNING: undefined label: extension_envoy.filters.network.sip_proxy
/tmp/tmpav26zfxu/generated/rst/intro/arch_overview/security/secpos_requires_trusted_downstream_and_upstream.rst:15: WARNING: undefined label: extension_envoy.filters.network.sip_proxy
/tmp/tmpav26zfxu/generated/rst/intro/arch_overview/security/secpos_requires_trusted_downstream_and_upstream.rst:15: WARNING: undefined label: extension_envoy.filters.network.sip_proxy

@dorisd0102 dorisd0102 closed this Sep 17, 2021
@dorisd0102 dorisd0102 reopened this Sep 17, 2021
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Comment thread contrib/sip_proxy/filters/network/source/: Outdated
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@dorisd0102 dorisd0102 changed the title sip_proxy: SIP protocol support in envoy WIP: sip_proxy: SIP protocol support in envoy Sep 24, 2021
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@dorisd0102 dorisd0102 changed the title WIP: sip_proxy: SIP protocol support in envoy sip_proxy: SIP protocol support in envoy Sep 25, 2021
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM. Please merge main and remove all of the generated_api_shadow files.

/wait

Comment thread source/common/common/logger.h Outdated
FUNCTION(udp) \
FUNCTION(wasm)
FUNCTION(wasm) \
FUNCTION(sip)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a good solution for this right now but I would prefer to not add a new logging category for a contrib extension. Please remove this and use something generic for now like filter, udp, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

Comment on lines +62 to +63
// Sip proxy filter
const std::string SipProxy = "envoy.filters.network.sip_proxy";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to remove this file, so please don't add this and just inline the string where needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Move to other file.

Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
Signed-off-by: Mingling <mingling.ding01@gmail.com>
@dorisd0102
Copy link
Copy Markdown
Contributor Author

@mattklein123 Many thanks for your comments. All are addressed, please help to have a further review. Thanks.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit dc1130d into envoyproxy:main Sep 28, 2021
@mattklein123
Copy link
Copy Markdown
Member

@dorisd0102 one of these tests fails for me locally:

[ RUN      ] SipConnectionManagerTest.UpstreamData
TestRandomGenerator running with seed 1308786792
[2021-09-28 17:11:03.443][12][error][connection] [contrib/sip_proxy/filters/network/source/conn_manager.cc:316] sip response application error: MockResponseDecoderAppException
[2021-09-28 17:11:03.444][12][error][connection] [contrib/sip_proxy/filters/network/source/conn_manager.cc:323] [C6] sip response error: MockResponseDecoderEnvoyException
[2021-09-28 17:11:03.444][12][critical][assert] [contrib/sip_proxy/filters/network/source/conn_manager.cc:155] assert failure: metadata_ != nullptr.
[2021-09-28 17:11:03.444][12][critical][backtrace] [./source/server/backtrace.h:104] Caught Aborted, suspect faulting address 0x3e80000000c
[2021-09-28 17:11:03.444][12][critical][backtrace] [./source/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2021-09-28 17:11:03.444][12][critical][backtrace] [./source/server/backtrace.h:92] Envoy version: 0/1.20.0-dev/test/DEBUG/BoringSSL
[2021-09-28 17:11:03.476][12][critical][backtrace] [./source/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0x3dc7d2c]
[2021-09-28 17:11:03.476][12][critical][backtrace] [./source/server/backtrace.h:96] #1: __restore_rt [0x7fb1b0a6b980]
[2021-09-28 17:11:03.509][12][critical][backtrace] [./source/server/backtrace.h:96] #2: Envoy::Extensions::NetworkFilters::SipProxy::SipConnectionManagerTest::upstreamDataTest() [0x21969ec]
[2021-09-28 17:11:03.541][12][critical][backtrace] [./source/server/backtrace.h:96] #3: Envoy::Extensions::NetworkFilters::SipProxy::SipConnectionManagerTest_UpstreamData_Test::TestBody() [0x21900d8]
[2021-09-28 17:11:03.572][12][critical][backtrace] [./source/server/backtrace.h:96] #4: testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x52cfbd4]
[2021-09-28 17:11:03.604][12][critical][backtrace] [./source/server/backtrace.h:96] #5: testing::internal::HandleExceptionsInMethodIfSupported<>() [0x52be40b]
[2021-09-28 17:11:03.636][12][critical][backtrace] [./source/server/backtrace.h:96] #6: testing::Test::Run() [0x52a9de3]
[2021-09-28 17:11:03.667][12][critical][backtrace] [./source/server/backtrace.h:96] #7: testing::TestInfo::Run() [0x52aa7b7]
[2021-09-28 17:11:03.699][12][critical][backtrace] [./source/server/backtrace.h:96] #8: testing::TestSuite::Run() [0x52aaf6c]
[2021-09-28 17:11:03.730][12][critical][backtrace] [./source/server/backtrace.h:96] #9: testing::internal::UnitTestImpl::RunAllTests() [0x52b6f0f]
[2021-09-28 17:11:03.762][12][critical][backtrace] [./source/server/backtrace.h:96] #10: testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x52d39a4]
[2021-09-28 17:11:03.794][12][critical][backtrace] [./source/server/backtrace.h:96] #11: testing::internal::HandleExceptionsInMethodIfSupported<>() [0x52c06bb]
[2021-09-28 17:11:03.825][12][critical][backtrace] [./source/server/backtrace.h:96] #12: testing::UnitTest::Run() [0x52b6ab3]
[2021-09-28 17:11:03.857][12][critical][backtrace] [./source/server/backtrace.h:96] #13: RUN_ALL_TESTS() [0x39e8421]
[2021-09-28 17:11:03.888][12][critical][backtrace] [./source/server/backtrace.h:96] #14: Envoy::TestRunner::RunTests() [0x39e7c10]
[2021-09-28 17:11:03.919][12][critical][backtrace] [./source/server/backtrace.h:96] #15: main [0x39e5dee]
[2021-09-28 17:11:03.920][12][critical][backtrace] [./source/server/backtrace.h:96] #16: __libc_start_main [0x7fb1b0689bf7]
================================================================================

Any quick idea what the problem might be? Otherwise I'm going to revert.

@mattklein123
Copy link
Copy Markdown
Member

Oh hmm I think we only do release builds in CI, so this I'm guessing is a fastbuild/debug issue. @dorisd0102 can you test and fix on fastbuild/debug? Otherwise I will revert until fixed, thanks.

@durd07
Copy link
Copy Markdown
Member

durd07 commented Sep 29, 2021

Hi @mattklein123, I create another PR to solve this UT issue, #18303.
Please help to review and merge it.
Thanks.

mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 29, 2021
* main: (114 commits)
  kafka: add header support to mesh-filter (envoyproxy#18248)
  rbac: add support for upstream ip policy. (envoyproxy#17645)
  SIPProxy BUGFIX UT failure for fastbuild/debug (envoyproxy#18303)
  quic: updating goaway code (envoyproxy#18291)
  various tiny fixes (envoyproxy#18287)
  dns cache: remove assert at this layer (envoyproxy#18301)
  [ext_authz]: ext_authz filter unit test that use real threading (envoyproxy#17742)
  signal action: fully disable sigaltstack on Apple (envoyproxy#18299)
  Add missing dependencies (envoyproxy#18297)
  ext_proc: Pass stream_info to gRPC streams (envoyproxy#18190)
  use clang 12 (envoyproxy#18220)
  Update PR template to include the "Fixes commit" message when reverting or fixing bad commits (envoyproxy#18298)
  [test] Fixing integration test to cleanup cleanly (envoyproxy#18293)
  test: moving grpc bridge tests out of core directory (envoyproxy#18227)
  runtime: disable deprecated extensions names by default (envoyproxy#18239)
  quiche: updating deps (envoyproxy#18272)
  sip_proxy: SIP protocol support in envoy (envoyproxy#18039)
  http: add core retry policy to route retry policy conversion utility (envoyproxy#17803)
  build: updating stale visibility (envoyproxy#18278)
  alternate_protocols_cache: Impose a max size limit on the alternate protocols cache (envoyproxy#18258)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@durd07 durd07 deleted the sip-proxy branch February 8, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants