envoy: creating a stripped main base without validation server#24294
envoy: creating a stripped main base without validation server#24294alyssawilk merged 15 commits intoenvoyproxy:mainfrom
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
@abeyad this one's still a draft so please do ignore for now |
b31fc73 to
eae8eb8
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
eae8eb8 to
f491191
Compare
|
/retest |
|
Retrying Azure Pipelines: |
|
I can confirm that this removes some stuff from the iOS framework: # Before
$ size Envoy.framework/Envoy
508M Envoy.framework/Envoy
$ nm Envoy.framework/Envoy | grep Server14validateConfig
U __ZN5Envoy6Server14validateConfigERKNS0_7OptionsERKNSt3__110shared_ptrIKNS_7Network7Address8InstanceEEERNS0_16ComponentFactoryERNS_6Thread13ThreadFactoryERNS_10Filesystem8InstanceE
0000000000005548 T __ZN5Envoy6Server14validateConfigERKNS0_7OptionsERKNSt3__110shared_ptrIKNS_7Network7Address8InstanceEEERNS0_16ComponentFactoryERNS_6Thread13ThreadFactoryERNS_10Filesystem8InstanceE
00000000000c6128 b __ZZN5Envoy6Server14validateConfigERKNS0_7OptionsERKNSt3__110shared_ptrIKNS_7Network7Address8InstanceEEERNS0_16ComponentFactoryERNS_6Thread13ThreadFactoryERNS_10Filesystem8InstanceEE7flogger
000000000008c6d8 s l___func__._ZN5Envoy6Server14validateConfigERKNS0_7OptionsERKNSt3__110shared_ptrIKNS_7Network7Address8InstanceEEERNS0_16ComponentFactoryERNS_6Thread13ThreadFactoryERNS_10Filesystem8InstanceE
# After
$ size Envoy.framework/Envoy
504M Envoy.framework/Envoy
$ nm Envoy.framework/Envoy | grep Server14validateConfigI can do a more thorough review when it's ready. |
|
Per IM, this requires some scrutiny of how we use the MainCommon/MainCommonBase APIs internally to ensure we have a path to being successful in the new model. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
KBaichoo
left a comment
There was a problem hiding this comment.
Few questions: Can you shed light on why we're splitting these e.g. what's the problem we're trying to solve? What the reason is we're removing validation from MainCommonBase? Thank you.
source/exe/main_common_base.h
Outdated
| Server::Configuration::Initial& config) override; | ||
| }; | ||
|
|
||
| class MainCommonBase { |
There was a problem hiding this comment.
Can this class be documented? e.g. especially differences between MainCommon and MainCommonBase
|
/wait |
e1c747b
|
/wait on cI |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/exe/BUILD
Outdated
| srcs = ["main_common.cc"], | ||
| hdrs = ["main_common.h"], | ||
| deps = [ | ||
| "//source/server/config_validation:server_lib", |
There was a problem hiding this comment.
cc @phlax I think this version should not have passed format checks. Did we lose buildifier on our format checks?
There was a problem hiding this comment.
i believe it works the same as it always did
afaict the queue was cancelled before the check was run
it the ./tools/code_format/check_format.py script that runs it i think
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
Signed-off-by: alyssawilk <alyssar@chromium.org>
|
@KBaichoo Ci should be sorted, PTAL! |
Commit Message: Additional Description: Register default listener. Envoy Mobile crashes in optimized builds without this change. The crash was introduced in #24294. The objc hello world app CI job was disabled in #24363 to make main green again. Risk Level: Low Testing: Docs Changes: Release Notes: 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):]
Splitting main common base into a third class, to allow for a main base without the validation server
Risk Level: Medium (added fast fail for main common base validate to mittigate)
Testing: updated tests
Docs Changes: n/a
Release Notes: did not add