Skip to content

dns: renumber dns filter fields for wire compatibility (#17943)#18018

Merged
mathetake merged 1 commit intoenvoyproxy:release/v1.19from
jpeach:backport-dns-filter-compat
Oct 21, 2021
Merged

dns: renumber dns filter fields for wire compatibility (#17943)#18018
mathetake merged 1 commit intoenvoyproxy:release/v1.19from
jpeach:backport-dns-filter-compat

Conversation

@jpeach
Copy link
Contributor

@jpeach jpeach commented Sep 8, 2021

Commit Message:

Field 2 in the DnsFilterConfig message was released in Envoy 1.18,
and repurposed in Envoy 1.19. Renumber the field, while leaving a
compatibility field so that control planes can gracefully migrate to a
subsequent 1.19 release.

Risk Level: Medium. Note that this breaks wire compatibility with 1.19.0 and 1.19.1 in order to fix wire compatibility with 1.18.
Testing: Unit tests.
Docs Changes: None.
Release Notes: Included.
Fixes #17921.

This is a backport of #17943 for the 1.19.2 release.

)

Field 2 in the `DnsFilterConfig` message was released in Envoy 1.18,
and repurposed in Envoy 1.19. Renumber the field, while leaving a
compatibility field so that control planes can gracefully migrate to a
subsequent 1.19 release.

This fixes envoyproxy#17921.

Signed-off-by: James Peach <jpeach@apache.org>
(cherry picked from commit ff9704e)
@repokitteh-read-only
Copy link

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: #18018 was opened by jpeach.

see: more, trace.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 8, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #18018 (comment) was created by @jpeach.

see: more, trace.

@suniltheta
Copy link
Contributor

Do we need to back port this fix #17107 so that CI pass?

Comment on lines +58 to +69
// This field was used for `dns_resolution_config` in Envoy 1.19.0 and
// 1.19.1.
// Control planes that need to set this field for Envoy 1.19.0 and
// 1.19.1 clients should fork the protobufs and change the field type
// to `DnsResolutionConfig`.
// Control planes that need to simultaneously support Envoy 1.18.x and
// Envoy 1.19.x should avoid Envoy 1.19.0 and 1.19.1.
//
// [#not-implemented-hide:]
repeated config.core.v3.Address upstream_resolvers = 2
[deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just reserve this number and field name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that there is a control plane that will consume these protobufs and have Envoy 1.18 and 1.19 clients. That's the case for Kuma (though IIUC we will end up consuming this change from Envoy master via the sync to go-control-plane).

Anyway, reserving the field fixes the envoy side in this branch, but the type is still needed so that a control plane can use this proto definition with 1.18.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 13, 2021

@lizan I think the presumbit test need #17767

@alyssawilk
Copy link
Contributor

/assign-from @envoyproxy/api-shepherds

@repokitteh-read-only
Copy link

@envoyproxy/api-shepherds assignee is @lizan

🐱

Caused by: a #18018 (comment) was created by @alyssawilk.

see: more, trace.

@markdroth
Copy link
Contributor

/lgtm api

@mattklein123
Copy link
Member

cc @mathetake for stable maintainer review.

@htuch
Copy link
Member

htuch commented Oct 20, 2021

Friendly ping @mathetake

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late-- LGTM

@mathetake mathetake self-assigned this Oct 20, 2021
@mathetake
Copy link
Member

compile_time_options has been broken since before 1.19.1 was released so I think it is harmless. I will merge this one.

@mathetake mathetake merged commit 25dbd02 into envoyproxy:release/v1.19 Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants