Adding ECDS config dump support.#23902
Conversation
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/assign @kyessenov @adisuissa |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
Overall the API LGTM.
2 high level comments/questions:
- Memory consumption could be non-negligible here. Is there anything we can do to only impact those that want to use this feature?
- Should there be a way to filter for a specific ECDS config (e.g., using https://www.envoyproxy.io/docs/envoy/latest/operations/admin#get--config_dump?name_regex=)?
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…lter config dump case Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…the for loop in the dumpEcdsConfig() function Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Merge branch 'main' of https://github.com/envoyproxy/envoy into ecds_config_dump Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@envoyproxy/api-shepherds PTAL |
|
/assign @yanavlasov |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Overall code changes LGTM.
I would suggest rethinking about the tests, as they seem to be complex and validate multiple things. See if you can make them more hermetic by targeting specific validations.
/lgtm api
test/integration/listener_extension_discovery_integration_test.cc
Outdated
Show resolved
Hide resolved
|
/retest |
|
Retrying Azure Pipelines: |
…config_dump Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
@kyessenov @adisuissa The PR had been updated. Please approve again if no further comments. |
|
@yanavlasov PTAL |
|
I believe this was the other half of breaking coverage for main |
This reverts commit 3752119.
When working on #23902 , we found the code : UpstreamHttpFilterConfigProviderManagerImpl in config_discovery_impl.h is not tested, which is causing the test coverage in that file is low. |
|
Coverage passed on main before your PR, and your PR made additions to a file. I have never seen this then fail without the reason being your new code lowered overall coverage. I'm sorry about this mix up and happy to point you at the coverage results when you try to reinstate it and help you sort out how to do it. The greater problem was this PR was unintentionally merged without the correct approvals so we have to fix that up as well. |
|
yeah looks like you were penalized for 2 comments (stupid sorry) but also didn't test an early return: https://storage.googleapis.com/envoy-postsubmit/main/coverage/source/common/filter/config_discovery_impl.h.gcov.html |
|
Thanks for sharing the coverage link. Yes, let me refactor the code to remove that early return. This is done in most other config dump code. Also removing those two comments line to increase the coverage. |
|
Hi, @alyssawilk , I quickly created this PR to address the test flaky issue in config_discovery_impl.h: #24362 |
History: ECDS config dump is first committed by : envoyproxy#23902. There are some tests issues which is fixed by: envoyproxy#24362 This PR is to: revert envoyproxy#24354, at same time add a couple lines added in envoyproxy#24362, to re-commit ECDS config dump functionality. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…verted by (envoyproxy#24354) This reverts commit c5d6160. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
5 participants
For issue: #20086
Signed-off-by: Yanjun Xiang yanjunxiang@google.com
Commit Message:
Additional Description:
Risk Level:
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:]