Skip to content

extensions: update straggler v2 extensions to v3.#14907

Merged
lizan merged 7 commits intoenvoyproxy:mainfrom
htuch:missing-v3-configs
Feb 11, 2021
Merged

extensions: update straggler v2 extensions to v3.#14907
lizan merged 7 commits intoenvoyproxy:mainfrom
htuch:missing-v3-configs

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Feb 2, 2021

  • Add v3 equivalents of v2 configs that were included in v3 due to no
    transitive deprecation. This increases consistency and reduces user
    confusion. We will continue to support these straggler v2 configs
    beyond the v2 turndown due to the late addition of v3 counterparts,
    special case code is added to utility.cc to handle this.

  • There were two extensions, //envoy/config/cluster/redis and
    //envoy/config/retry/previous_priorities, that for some reason were
    not upgraded to use v3 config. This is now fixed and I've grepped for
    other v2 in //source, none remain.

Risk level: Medium (changes to extension config types and deprecated
config handling).
Testing: Additional unit test added for utility.cc handling, upgraded
configs to v3 for other tests.

Fixes #14735
Fixes #12841

Signed-off-by: Harvey Tuch htuch@google.com
Co-authored-by: Abhay Narayan Katare abhay.katare@india.nec.com

* Add v3 equivalents of v2 configs that were included in v3 due to no
  transitive deprecation. This increases consistency and reduces user
  confusion. We will continue to support these straggler v2 configs
  beyond the v2 turndown due to the late addition of v3 counterparts,
  special case code is added to utility.cc to handle this.

* There were two extensions, //envoy/config/cluster/redis and
  //envoy/config/retry/previous_priorities, that for some reason were
  not upgraded to use v3 config. This is now fixed and I've grepped for
  other v2 in //source, none remain.

Risk level: Medium (changes to extension config types and deprecated
  config handling).
Testing: Additional unit test added for utility.cc handling, upgraded
  configs to v3 for other tests.

Fixes envoyproxy#14735
Fixes envoyproxy#12841

Signed-off-by: Harvey Tuch <htuch@google.com>
Co-authored-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14907 was opened by htuch.

see: more, trace.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 2, 2021

@ankatare this lands #12841, thanks for your help.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 2, 2021

@snowp if you can look at the retry related extension bits it would be a helpful sanity check.

@ankatare
Copy link
Copy Markdown
Contributor

ankatare commented Feb 2, 2021

@htuch Thanks :)

@lizan
Copy link
Copy Markdown
Member

lizan commented Feb 2, 2021

This is great, can you fix CI?

@lizan lizan added the waiting label Feb 2, 2021
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 4, 2021

@lizan this should be ready for review; the CI fails that remain are unrelated flakes.

Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

/wait

syntax = "proto3";

package envoy.config.retry.omit_canary_hosts.v3;
package envoy.extensions.retry.host.omit_canary_hosts.v3;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

won't this break wire compatibility?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I merged this one only a short while back and it's never been used by Envoy, so we can do this.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 7, 2021

@lizan other thoughts? Would be great to merge this soon.

@htuch htuch mentioned this pull request Feb 7, 2021
lizan
lizan previously approved these changes Feb 9, 2021
@repokitteh-read-only repokitteh-read-only bot removed the api label Feb 9, 2021
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 10, 2021

@lizan can I get another rubber stamp? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PreviousPrioritiesConfig Type Not Found Migrate FixedHeap resource monitor to api v3

4 participants