[http] Initial codec splitting with test parametrization

[asraa]

Commit Description:
This just introduces plumbing and clones codecs for a legacy versions.
There is no changes to the codecs at this point (the files are just duplicated and the namespace Legacy is used). Users can set which codecs are used at runtime via the feature new_codec_behavior. This will be the case for O(4-6 weeks) while the change to new no-exception codecs are pushed (see issue linked) to the new codecs.

Format scripts check that h/1 header and source, h/2 header and source, and h/2 codec tests are kept in sync to enforce that devs who make changes to one codec port them to the old one. Devs will need to update the diff if they are making a change that exists in one and not the other. The format script will alert them on what to change in the diff script.

Risk Level: Medium. No-op for behavior, but high because it branches and splits codecs.
Testing:

• The H/1 test is parametrized for both new and legacy codec
• The H/2 test is built twice, in new and legacy mode (legacy mode is triggered with a --runtime-disable-for-test flag)
• format enforces the tests stay in sync, although this may not be needed since the tests run in both modes.
• Integration tests run over all their combinations of upstream/downstream legacy and new codec versions in compile_time_options.

Release Notes: Added release note
Part of #10878

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10591 was opened by asraa.

see: more, trace.

[asraa]

Some questions:

• Defaults should still be the existing codecs, which makes me think I should flip LEGACY_HTTP[12] to NOEXCEPT_HTTP[12] or something that would make it harder for users to choose the WIP/new codecs.
• The grpc parametrized integration tests require more plumbing to get to work with the codecs. It makes me think I should just make a LegacyHttpIntegrationTest class.

[mattklein123]

@asraa very quick comment: I don't think we should use config for picking between legacy/exception. It's an implementation detail. Can we just use runtime like we typically do for this? I think the default should be the new code.

[asraa]

@asraa very quick comment: I don't think we should use config for picking between legacy/exception. It's an implementation detail. Can we just use runtime like we typically do for this? I think the default should be the new code.

Yeah will do -- I'll remove it from config and add a runtime switch with default Legacy. Should upstream/downstream be separate switch? If there's just one "use_legacy" switch, I'd run tests with legacy on/off, not with different combos of upstream/downstream legacy choices. I'm happier doing that. Much easier to plumb

[mattklein123]

If there's just one "use_legacy" switch, I'd run tests with legacy on/off, not with different combos of upstream/downstream legacy choices. I'm happier doing that. Much easier to plumb

Yes +1, let's just keep it simple. Also, FWIW, I think the default should be new/no exceptions vs. legacy. I don't really think these changes are scary enough to warrant default legacy, but I don't feel super strongly about it if you and @alyssawilk feel otherwise.

[alyssawilk]

I think as long as people can opt out and we warn there's a series of high risk changes (stick in the release notes as we add the runtime flag) coming we can default on. We can get more conservative if the tests prove to not be sufficient for this sort of refactor change.

[asraa]

Gotcha! Will plumb that through and push again with the runtime flip.

[htuch]

+1 on runtime vs. config.

[asraa]

Before I start -- instead of TEST_P'ing over a bool to setLegacyCodec() in each integration test (and also codec_client_test/conn_pool_test), should I just create duplicate bazel targets that would define a macro that conditionally calls setLegacyCodec() ?

[htuch]

@asraa how many tests files vs. individual TEST_F are we talking about?

[asraa]

In test/integration: 504 tests individual tests, 49 integration test classes that would need to be parametrized, 42 test files

Mostly worried for extensibility and requiring that anyone adding integration tests makes sure to parametrize it

[alyssawilk]

Hmmm. Brainstorming simpler options, what if we had the default option set in a #define, and ran all the tests is non-default mode in one of the builds (maybe the one that runs with deprecated options fatal), and then only do a few tests in both A and B modes? It would mean we didn't have comprehensive coverage (asan and tsan) for all integration tests, but they'd all run in both modes.

…

On Tue, Mar 31, 2020 at 2:40 PM asraa ***@***.***> wrote: In test/integration: 504 tests individual tests, 49 integration test classes that would need to be parametrized, 42 test files — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#10591 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPIUCOAYRO6VNHYMVV3RKI2LBANCNFSM4LXWE3RA> .

[asraa]

It would mean we didn't have comprehensive coverage (asan and tsan) for all integration tests, but they'd all run in both modes.

I like the idea of a running them with a separate build that runs with a --define use_legacy=true, but am worried about not having any asan coverage. Maybe we can run just //test/integration/... with asan here?

envoy/ci/do_ci.sh

Line 232 in 016e92d

echo "Building and testing ${TEST_TARGETS}"

I can parametrize over the other two tests since they're one and done things (codec_client_test)

[alyssawilk]

SGTM, though @lizan or @htuch might have opinions on how to best do build / CI workarounds. I think ASAN and debug builds are most important (catch assertion failures and any memory issues) so I think if we do unit and integration tests for legacy for those we're good.

[asraa]

Merged master, fixed clang-tidy, and will check the diff in CI time with this commit.
Quick question: I just wrangled with an integration test error for an hour or two before I realized it was happening because of a merged change in the new codecs that wasn't back ported to the legacies. To stop this from happening all the time, is there an easy way to write a fix_format rule that does the diff of the codec_impl's and codec_impl_legacy's? There's an expected diff (in namespaces), but the line numbers would change with code additions

[alyssawilk]

LGTM, but let's definitely get a non-googler sign off on this one! :-)

[asraa]

@mattklein123 When you have a chance, could you please take a look? Or any other non-googler maintainer :)

Summary of updates:

• integration tests parametrized and run in legacy mode in compile_time_options
• h/1 codec_impl_test is parametrized on codec type
• h/2 codec_impl_test is also parametrized, but the target is duplicated and built with a test flag for runtime override
• no-op everywhere else, codec implementations are cloned, default new codecs
• format still enforces the devs change or make updates to the golden diffs, but that may be unncessary now that the tests are parametrized
• clang_tidy fails on existing issue The LinkedObject::moveIntoList usage is unsafe #11691

[mattklein123]

LGTM!

[yanavlasov]

Clang tidy seems to be off, flagging gtest macros with invalid casing.