Put dns_resolution_config field in dns_filter.proto as deprecated

[yanjunxiang-google]

Problem Description:
This PR is the code changes to deprecate dns_resolution_config field in dns_filter.proto for issue: #18053. This is a follow up PR of #17479.

Solution:

1. Put dns_resolution_config field in dns_filter.proto as deprecated.
2. This deprecation is causing CI compile_time_options test scripts failure, due to dns_filter_test.cc and dns_filter_integration_test.cc have dns_resolution_config in the config YAML file, and also called loadFromYamlAndValidate(), which will fail if --define=deprecated_features=disabled is included in the test command.
3. Replace dns_resolution_config in the config YAML file with typed_dns_resolution_config.

Build:
passed

Testing:
passed

Release Notes:
N/A

Issues: deprecate dns_resolution_config #18053

Fix#18053

Signed-off-by: Yanjun Xiang [email protected]

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:]

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18649 was opened by yanjunxiang-google.

see: more, trace.

[yanjunxiang-google]

For reviewers:

The changes replaced dns_resolution_config with the newly added typed_dns_resolver_config. The reasons to not test both of them are:

1. dns_filter code are using typed_dns_resolver_config to create DNS resolver.
2. if dns_resolution_config is in the configuration, the code converts them into typed_dns_resolver_config before creating DNS resolver.
3. The dns_resolution_config to typed_dns_resolver_config conversion part is now done by a common utility function implemented in dns_factory.[h|cc], and this conversion logic is heavily tested in dns_factory_test.cc, cluster_manager_impl_test.cc, .... Thus it's unnecessary to test them again here also wrap the tests with DEPRECATED_FEATURE_TEST().

[yanjunxiang-google]

/assign @yanavlasov @htuch

[yanjunxiang-google]

/assign @suniltheta @jpeach

[repokitteh-read-only]

neither of @suniltheta, @jpeach can be assigned to this issue.

🐱

Caused by: a #18649 (comment) was created by @yanjunxiang-google.

see: more, trace.

[yanavlasov]

/wait

[htuch]

/lgtm api

[suniltheta]

Few minor changes.

[yanjunxiang-google]

Thanks! Adding a new test like 2) make sense to me. Just pushed the new changes to the PR.

…

On Wed, Oct 20, 2021 at 12:25 PM Sunil Narasimhamurthy < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/extensions/filters/udp/dns_filter/dns_filter_test.cc <#18649 (comment)>: > @@ -2194,7 +2207,7 @@ stat_prefix: "my_prefix" } // test typed_dns_resolver_config exits which overrides dns_resolution_config. -TEST_F(DnsFilterTest, TypedDnsResolverConfigExist) { +TEST_F(DnsFilterTest, DEPRECATED_FEATURE_TEST(TypedDnsResolverConfigExist)) { Sure, I get it now. In future when we remove the support for deprecated field dns_resolution_config we should not accidentally remove this test case entirely. I am thinking we can do 1 of these now: 1. Add a comment saying when we remove the support for deprecated field dns_resolution_config entirely just remove the tag DEPRECATED_FEATURE_TEST and remove dns_resolution_config from the yaml config. OR 2. Rename this existing test DEPRECATED_FEATURE_TEST (TypedDnsResolverConfigExist) --> DEPRECATED_FEATURE_TEST (TypedDnsResolverConfigOverrideDnsResolutionConfig). Add another unit test with name TypedDnsResolverConfigExist where we don't include field dns_resolution_config in the yaml. What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASZIHLGXH3BMSKMO4IT6IJTUH3UPVANCNFSM5GDX6EJQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[KBaichoo]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18649 (comment) was created by @KBaichoo.

see: more, trace.

[yanjunxiang-google]

/assign @mattklein123

[mattklein123]

Thanks!