V2 to v3 fragment #11213

ankatare · 2020-05-15T11:26:16Z

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

Commit Message: fragment changes from v2 to v3 API
Additional Description:
Risk Level:Low
Testing: integration and format testing
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #10843
[Optional Deprecated:]

htuch

Thanks for this major cleanup. The main thing I think we should do before shipping is ensuring that the parseFooFromV3Yaml rejects v2 configs.
/wait

test/common/upstream/cluster_manager_impl_test.cc

test/common/upstream/utility.h

ankatare · 2020-05-19T14:10:27Z

@htuch Thanks for your review comments. I will update code as per your suggestion. Would be helpful if you could provide some code ref for the same.

htuch · 2020-05-20T01:48:30Z

@ankatare you want to invoke MessageUtil::loadFromYaml with do_boosting set to false.

ankatare · 2020-05-21T11:16:54Z

@htuch ok, I will do that. Thanks for Help !!!

ankatare · 2020-05-26T12:21:14Z

@htuch : please see possible code changes and it's implications.

File: test/test_common/utility.h
Function Change:
static void loadFromYaml(const std::string& yaml, Protobuf::Message& message,
bool preserve_original_type = false) {
MessageUtil::loadFromYaml(yaml, message, ProtobufMessage::getStrictValidationVisitor(),false);
if (!preserve_original_type) {
Config::VersionConverter::eraseOriginalTypeInformation(message);
}
}

Implications : Almost every test cases under test/.. will be affected due to this change and will be required some rework.

Please suggest if it is good to go. .

htuch · 2020-05-27T00:01:11Z

@ankatare why did you go with type erasure vs. avoiding boosting in the first place?

ankatare · 2020-05-28T11:15:04Z

@htuch thanks for your quick comments.
Do you means to set "avoid do_boosting" from inside
function : MessageUtil::loadFromYam () {}. as i see execution of actual logic is happening inside MessageUtil::loadFromJson () {}

htuch · 2020-05-29T23:14:40Z

@ankatare see

envoy/source/common/protobuf/utility.h

Line 224 in 582eb0e

bool do_boosting = true);

, the ability to control do_boosting already exists. I think this should be plumbed through to the test wrappers.

test/extensions/health_checkers/redis/config_test.cc

ankatare · 2020-05-30T09:49:40Z

@htuch Thanks for comments. I will get back as soon as possible

ankatare · 2020-06-03T17:54:50Z

@htuch Hi, functionality of do_boosting is already implemented in https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L222.
In my opinion i just need to set value of do_boosting to false (to avoid boosting) here and use in the test codes ... seems it was written to boost V2 config etc. Please let me know if my understanding is not correct here.

htuch · 2020-06-04T00:19:45Z

@ankatare yes, when you want to force it to parse as v2, you need to set do_boosting to false.

ankatare · 2020-06-08T16:35:57Z

@htuch while applying avoid-boosting ( do_boosting = false ) in test code, 2 test cases are failing. (remaining are ok)
it seems for both to get passed boosting is required.
they are below test cases
https://github.com/envoyproxy/envoy/blob/master/test/common/upstream/cluster_manager_impl_test.cc#L419
and
https://github.com/envoyproxy/envoy/blob/master/test/common/upstream/cluster_manager_impl_test.cc#L539
please suggest way forward to proceed as we can't change exception output ?

ankatare · 2020-06-08T16:37:23Z

@htuch or suggest from whom to take help.

htuch · 2020-06-09T23:25:50Z

@ankatare are you trying to treat these examples as v2 or v3?

ankatare · 2020-06-09T23:42:55Z

@htuch Thanks for your response.
seems, these test cases using v3 config only.

htuch · 2020-06-09T23:52:55Z

@ankatare and you are saying that when you parse them as v3, with do_boosting = false, this fails?

ankatare · 2020-06-10T00:01:37Z

@htuch yes.

htuch · 2020-06-10T00:07:31Z

You might want to run the test with --test_arg="-l trace" and see if there is some useful debug message. It should definitely be possible to parse v3 config as v3 without any boosting.

ankatare · 2020-06-10T00:11:46Z

@htuch ok. Let me check. Will get back ASAP

ankatare · 2020-06-10T00:23:53Z

@htuch see debug logs for
https://github.com/envoyproxy/envoy/blob/master/test/common/upstream/cluster_manager_impl_test.cc#L419
test/common/upstream/cluster_manager_impl_test.cc:397: Failure
Value of: std::string(e.what())
Expected: is equal to 0x25cf98 pointing to "cluster: LB policy hidden_envoy_deprecated_ORIGINAL_DST_LB is not valid for Cluster type STATIC. 'ORIGINAL_DST_LB' is allowed only with cluster type 'ORIGINAL_DST'"
Actual: "Protobuf message (type envoy.config.bootstrap.v3.Bootstrap reason INVALID_ARGUMENT:(static_resources.clusters[0].lb_policy): invalid value "original_dst_lb" for type TYPE_ENUM) has unknown fields"
Stack trace:
0x15b9dd1: Envoy::Upstream::(anonymous namespace)::ClusterManagerImplTest_OriginalDstLbRestriction2_Test::TestBody()
0x3816404: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
0x38079be: testing::internal::HandleExceptionsInMethodIfSupported<>()
0x37f1c13: testing::Test::Run()
0x37f27fd: testing::TestInfo::Run()
... Google Test internal frames ...

[ FAILED ] ClusterManagerImplTest.OriginalDstLbRestriction2 (201 ms)

ankatare · 2020-06-10T00:27:26Z

@htuch following code is executing. i checked with debug output
https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.cc#L310

htuch · 2020-06-10T22:53:21Z

@ankatare that is v2 config if it's using ORIGINAL_DST_LB, which was deprecated. Should this be a fragment you hand convert to v3?

ankatare · 2020-06-10T23:30:31Z

@htuch yeah, while looking at this particular field it seems V2 config and it is deprecated as per doc https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cluster.proto#envoy-api-enum-cluster-lbpolicy.
and suggest to use "cluster_provided" as a value for lb_policy in V3.
but i have not modified it, as this particular test code is meant for original_dst_lb only for type static. ( looking at exception o/p )
please guide me as i am bit confused here :(

htuch · 2020-06-10T23:43:03Z

In this case, I think we should allow boosting, as the EXPECT_THROW is trying to show end-to-end what is expected. So, this is basically parse v2 with explicit boosting.

ankatare · 2020-06-10T23:46:12Z

@htuch Thanks a lot for this help. I will update code and get back ASAP.

Signed-off-by: Abhay Narayan Katare <[email protected]>

ankatare · 2020-07-01T11:27:42Z

@htuch need help to understand this build error , things were fine in my local env. i posted this issue in envoy-ci channel as well.

ankatare · 2020-07-01T14:38:29Z

@htuch please review changes. i incorporated all of your comments.

htuch

Thanks. Looks like good progress in some places, but I think we need to have:

Most of the v2 fragments updated to v3 (except for the deprecated feature tests).
Not set avoid_boosting in most place (except for the deprecated features tests).
/wait

test/common/tcp_proxy/tcp_proxy_test.cc

test/common/upstream/cluster_manager_impl_test.cc

test/extensions/filters/network/http_connection_manager/config_test.cc

ankatare · 2020-07-01T23:03:03Z

@htuch for some of the test cases i need to allow do_boosting (even they are not DEPRECATED) as the EXPECT_THROW is trying to show end-to-end what is expected. So, this is basically parse v2 with explicit boosting. same we discussed above.

ankatare · 2020-07-01T23:03:51Z

ankatare · 2020-07-01T23:06:57Z

@htuch for this comments i would say yes, it is a default parameter as false for loadFromYamlAndValidate . Since i needed to parse avoid_boosting as 4th parameter in function argument.

ankatare · 2020-07-01T23:11:53Z

@htuch for this case, to get it pass i just modified function arguments llist as in the another test cases i modified testutil::loadFromYamlAndValidate and it was calling it internally. so i am planning to change name one by one. after code is merged please suggest.

htuch

Looks good. One thing I'm curious about is we see almost no changes to the config; does this mean our config examples are v3 safe for the most part? @adisuissa was this basically what you also observed?

test/extensions/filters/network/http_connection_manager/config_test.cc

test/test_common/utility.h

ankatare · 2020-07-01T23:12:41Z

@htuch thanks a lot for all your time on review my code.

htuch · 2020-07-01T23:18:20Z

@ankatare no worries; where possible, can you reply using GitHub threads BTW? It's hard to follow conversations with screenshot (although my Slack conversations are fair game for screenshotting ;-) )

ankatare · 2020-07-01T23:22:22Z

@htuch really sorry and it is my bad. somehow i am not seeing reply on your individual inputs in my window. it's only a button of "resolve conversation".
i don't know how to enable option, please help.

adisuissa · 2020-07-02T01:06:23Z

Looks good. One thing I'm curious about is we see almost no changes to the config; does this mean our config examples are v3 safe for the most part? @adisuissa was this basically what you also observed?

Most configs were either v3 or v2+upgrade safe.

test/test_common/utility.h

source/common/protobuf/utility.h

ankatare · 2020-07-02T17:36:31Z

@htuch @adisuissa : thanks for your review comments, i will get back ASAP.

Signed-off-by: Abhay Narayan Katare <[email protected]>

ankatare · 2020-07-04T16:29:14Z

@htuch incorporated review comments. thanks for your time to review it.

ankatare · 2020-07-04T16:29:41Z

@adisuissa incorporated review comments. thanks for your time to review it.

htuch

LGTM, thanks! As discussed offline, @ankatare will work on some of the test cases I had questions around in the next PR.

Risk Level:Low Testing: integration and format testing Part of envoyproxy#10843 Signed-off-by: Abhay Narayan Katare <[email protected]> Signed-off-by: scheler <[email protected]>

jmarantz assigned htuch May 18, 2020

htuch reviewed May 18, 2020

View reviewed changes

test/common/upstream/cluster_manager_impl_test.cc Outdated Show resolved Hide resolved

test/common/upstream/utility.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 18, 2020

ankatare mentioned this pull request May 29, 2020

Convert cluster-related v2 API test fragments to v3 #11031

Merged

htuch reviewed May 29, 2020

View reviewed changes

test/extensions/health_checkers/redis/config_test.cc Outdated Show resolved Hide resolved

review comments incorporated

b41982e

Signed-off-by: Abhay Narayan Katare <[email protected]>

fixing previous build issues

787c510

Signed-off-by: Abhay Narayan Katare <[email protected]>

ankatare requested review from alyssawilk and mattklein123 as code owners July 1, 2020 07:52

htuch suggested changes Jul 1, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 1, 2020

htuch reviewed Jul 1, 2020

View reviewed changes

test/extensions/filters/network/http_connection_manager/config_test.cc Show resolved Hide resolved

test/test_common/utility.h Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the waiting label Jul 2, 2020

adisuissa reviewed Jul 2, 2020

View reviewed changes

test/test_common/utility.h Outdated Show resolved Hide resolved

adisuissa reviewed Jul 2, 2020

View reviewed changes

source/common/protobuf/utility.h Outdated Show resolved Hide resolved

review comments incorporated for htuch & adisuissa

c3608d2

Signed-off-by: Abhay Narayan Katare <[email protected]>

ankatare force-pushed the v2_to_v3_fragment branch from 109f9f4 to c3608d2 Compare July 4, 2020 10:49

htuch approved these changes Jul 6, 2020

View reviewed changes

htuch merged commit e73738f into envoyproxy:master Jul 6, 2020

ankatare deleted the v2_to_v3_fragment branch July 14, 2020 06:19

V2 to v3 fragment #11213

V2 to v3 fragment #11213

Conversation

ankatare commented May 15, 2020

htuch left a comment

Choose a reason for hiding this comment

ankatare commented May 19, 2020

htuch commented May 20, 2020

ankatare commented May 21, 2020

ankatare commented May 26, 2020

htuch commented May 27, 2020

ankatare commented May 28, 2020

htuch commented May 29, 2020

ankatare commented May 30, 2020

ankatare commented Jun 3, 2020

htuch commented Jun 4, 2020

ankatare commented Jun 8, 2020

ankatare commented Jun 8, 2020

htuch commented Jun 9, 2020

ankatare commented Jun 9, 2020 • edited Loading

htuch commented Jun 9, 2020

ankatare commented Jun 10, 2020

htuch commented Jun 10, 2020

ankatare commented Jun 10, 2020

ankatare commented Jun 10, 2020

ankatare commented Jun 10, 2020

htuch commented Jun 10, 2020

ankatare commented Jun 10, 2020

htuch commented Jun 10, 2020

ankatare commented Jun 10, 2020

ankatare commented Jul 1, 2020

ankatare commented Jul 1, 2020

htuch left a comment

Choose a reason for hiding this comment

ankatare commented Jul 1, 2020

ankatare commented Jul 1, 2020

ankatare commented Jul 1, 2020

ankatare commented Jul 1, 2020

htuch left a comment

Choose a reason for hiding this comment

ankatare commented Jul 1, 2020

htuch commented Jul 1, 2020

ankatare commented Jul 1, 2020

adisuissa commented Jul 2, 2020

ankatare commented Jul 2, 2020

ankatare commented Jul 4, 2020

ankatare commented Jul 4, 2020

htuch left a comment

Choose a reason for hiding this comment

ankatare commented Jun 9, 2020 •

edited

Loading