Put dns_resolution_config in dns_filter.proto files as deprecated

[yanjunxiang-google]

Title: Put dns_resolution_config in dns_filter.proto files as deprecated

Description:
With typed_dns_resolver_config being supported in #17479, dns_resolution_config in dns_filter.proto will be deprecated. And changing test cases in dns_filter_test.cc and dns_filter_integration_test.cc to use typed_dns_resolver_config.

[suniltheta]

I believe reserving this field dns_resolution_config would be less intrusive and easier approach. As we are still in Alpha on dns_filter.proto, no harm done on not supporting the configuration via reserved & not-implemented field dns_resolution_config.

The reason it should be reserved and not removed because v1.19.2 will not have typed_dns_resolver_config back ported and would still need to support dns_resolution_config, we don't want to break the compatibility between control plane supporting v1.19.2 & v1.20.0+.

I see that #17479 has already taken the step towards deprecating dns_resolution_config field, thanks.

[yanjunxiang-google]

Thanks for the information @suniltheta !

If dns_filter feature will be supported by Envoy and control-plane in the upcoming v1.19.2 & v.12.0+ , but typed_dns_resolver_config won't be there., can we put dns_resolution_config as "reserved"? Should we put it as "deprecated" and go through normal deprecation process?

[yanjunxiang-google]

BTW, we are not going to put dns_resolution_config as deprecated in #17479. Doing that is causing some CI scripts failure We are exploring the options how to deprecate it.
If we can put it as "reserved", and replace dns_resolution_config in dns_filter_test.cc/dns_filter_integration_test.cc with typed_dns_resolver_config , that seem to be a clean solution moving forward.

[suniltheta]

If we can put it as "reserved", and replace dns_resolution_config in dns_filter_test.cc/dns_filter_integration_test.cc with typed_dns_resolver_config , that seem to be a clean solution moving forward.

I totally agree with this.

Once these changes are done, then we can move the dns_filter.proto to non-alpha, as Matt suggested on #17943. Hopefully this can be done before v1.20 release.

cc @jpeach for your thoughts on if we can mark dns_resolution_config field 5 as reserved by v1.20 release.

[suniltheta]

Update: I redact what I said earlier about just reserving the field number 5 for dns_resolution_config.
@jpeach correct me if I am wrong here.

We still want control plane with v1.20.0 API to continue to support v1.18 & v1.19.2. So that means we cannot just reserve this field number 5. We still need to have dns_resolution_config marked as deprecated and annotated with [#not-implemented-hide:]. This will be similar to what we maintain for upstream_resolvers field. For ref comment on earlier PR.

Also we should replace dns_resolution_config in dns_filter_test.cc/dns_filter_integration_test.cc with typed_dns_resolver_config as mentioned by @yanjunxiang-google.

[yanjunxiang-google]

For your comments about "We still need to have dns_resolution_config marked as deprecated and annotated with [#not-implemented-hide:]", I don't see such deprecation and comment it in the dns_filter.proto file as below: https://github.com/envoyproxy/envoy/blob/a1a4595f2a2fc3be669ee6c81becb732dd6c030f/api/envoy/extensions/filters/udp/dns_filter/v3alpha/dns_filter.proto#L70 This field is implemented in https://github.com/envoyproxy/envoy/blob/a1a4595f2a2fc3be669ee6c81becb732dd6c030f/source/extensions/filters/udp/dns_filter/dns_filter.cc#L162 . I guess overall what you mean is that we can not remove or "reserve" *dns_resolution_config* field. We can only put it as "deprecated". If that's the case, since control planes are using it in some releases, we can not remove it from testing code, because we still need to test the code logic which is using this field. Is my understanding correct?

…

On Tue, Sep 14, 2021 at 3:16 PM Sunil Narasimhamurthy < ***@***.***> wrote: Update: I redact what I said earlier about just reserving the field number 5 for dns_resolution_config. @jpeach <https://github.com/jpeach> correct me if I am wrong here. We still want control plane with v1.20.0 API to continue to support v1.18 & v1.19.2. So that means we cannot just reserve this field number 5. We still need to have dns_resolution_config marked as deprecated and annotated with [#not-implemented-hide:]. This will be similar to what we maintain for upstream_resolvers field. For ref comment <#18018 (comment)> on earlier PR. Also we should replace dns_resolution_config in dns_filter_test.cc/dns_filter_integration_test.cc with typed_dns_resolver_config as mentioned by @yanjunxiang-google <https://github.com/yanjunxiang-google>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18053 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASZIHLARVKBCG4TRC7H47SDUB6NSHANCNFSM5DXY5CAQ> . 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>.

[yanjunxiang-google]

BTW, I am not too familiar with the release planning. But if we can make typed_dns_resolver_config support in the same release with dns_filter (since it is not released yet), then we can get rid of this dns_resolution_config, right?

[jpeach]

We still want control plane with v1.20.0 API to continue to support v1.18 & v1.19.2. So that means we cannot just reserve this field number 5. We still need to have dns_resolution_config marked as deprecated and annotated with [#not-implemented-hide:]. This will be similar to what we maintain for upstream_resolvers field. For ref comment on earlier PR.

Yeh, pretty sure that this is the best option. The go-control-plane project will release with an arbitrary snapshot of Envoy protobufs. Control planes downstream of that will then use those protobufs to program some version of Envoy. If you remove the field definition and simply reserve it, then someone will probably have to fork the protobufs to restore the definition.

IMHO, Envoy doesn't need to carry any code to implement or migrate the field. Control planes can be sophisticated enough to populate the right fields for the right Envoy versions.

BTW, I am not too familiar with the release planning. But if we can make typed_dns_resolver_config support in the same release with dns_filter (since it is not released yet), then we can get rid of this dns_resolution_config, right?

It seems fine to me to drop the implementation from Envoy (since it is alpha) and carry the old protobuf definitions for compatibility.

[yanjunxiang-google]

Thanks @suniltheta @jpeach for your info.

Here is my plan: 1) put dns_resolution_config in dns_filter.proto as deprecated. 2) Changing test cases in dns_filter_test.cc and dns_filter_integration_test.cc to use typed_dns_resolver_config so the Envoy CI can pass with this deprecation. 3) There is code added in #17479: dns_factory_test.cc which test this deprecated dns_resolution_config field in dns_filter.proto. Those code will be kept since they are not causing any CI scripts failure.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[yanjunxiang-google]

/wait

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[yanjunxiang-google]

This issue is fixed by : Put dns_resolution_config field in dns_filter.proto as deprecated #18649. Closing it.