Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release: flipping deprecated features to fatal-by-default #6509

Merged
merged 5 commits into from
Apr 10, 2019

Conversation

alyssawilk
Copy link
Contributor

Somewhat ironically, bind_to_port isn't currently flagged as deprecated but use_original_dst was, so manually extended its lifetime per discussion on #5355.

It wasn't clear to me if the tcp_proxy deprecated_v1 fields were also part of #5355 so tagging Piotr as a reviewer.

I'll envoy-announce once we've nailed down the fields and gotten approval from all.

Risk Level: High (first time using this process - it will likely cause problems for someone)
Testing: tests pass.
Docs Changes: n/a
Release Notes: Should we relnote these? They're already in DEPRECATED.md

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM but would like to discuss hosts a bit more with the group before we merge.

"envoy.deprecated_features.route.proto:enabled",
"envoy.deprecated_features.tcp_proxy.proto:deprecated_v1",
"envoy.deprecated_features.fault.proto:type",
"envoy.deprecated_features.cds.proto:hosts",
Copy link
Member

Choose a reason for hiding this comment

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

The docs aren't even done on this one IIRC. Should we punt this another release? cc @dio @htuch @envoyproxy/maintainers. (This one concerns me a lot from a complaining perspective and I'm still torn whether we should just leave it alone. Thoughts?)

Copy link
Member

Choose a reason for hiding this comment

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

After further discussions with @htuch this morning my current feeling is we should just undeprecate this. IMO it's not worth the pain. Would like to hear what others think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no objections for making hosts fatal-by-default, but un-deprecating sounds quite appealing, since Istio is still using hosts (though, I don't believe there are any blockers for migrating away from it).

Copy link

@andraxylia andraxylia Apr 8, 2019

Choose a reason for hiding this comment

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

"envoy.deprecated_features.tcp_proxy.proto:deprecated_v1" is used in Istio.

istio/istio#13156

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reasonable replacement for deprecated_v1 or is it still not ready? If the former, we can move forward with the deprecation per the policy. If not, we should undeprecate this field. cc @PiotrSikora

Copy link
Contributor

Choose a reason for hiding this comment

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

tcp_proxy.deprecated_v1 is not used in Istio, so we don't have objections removing that.

Having said that, Envoy still doesn't have a proper replacement for this feature, since no one implemented #4457 yet.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for @PiotrSikora. I think in that case we also need to undeprecate tcp_proxy.deprecated_v1 until we have a complete replacement.

"envoy.deprecated_features.config_source.proto:UNSUPPORTED_REST_LEGACY",
"envoy.deprecated_features.ext_authz.proto:use_alpha",
"envoy.deprecated_features.route.proto:enabled",
"envoy.deprecated_features.tcp_proxy.proto:deprecated_v1",
Copy link
Contributor

@lambdai lambdai Apr 8, 2019

Choose a reason for hiding this comment

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

Update: Istio is not using tcp_proxy.deprecated_v1

Copy link
Contributor

Choose a reason for hiding this comment

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

Link? I couldn't find any references to it last time I looked (few weeks ago).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't find it either. Update my original comment

@costinm
Copy link
Contributor

costinm commented Apr 8, 2019

tcp_proxy deprecated_v1 doesn't seem to be used - only has "Routes" which we're not using.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
PiotrSikora
PiotrSikora previously approved these changes Apr 9, 2019
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Deprecate all the features!

"envoy.deprecated_features.ext_authz.proto:use_alpha",
"envoy.deprecated_features.route.proto:enabled",
"envoy.deprecated_features.fault.proto:type",
"envoy.deprecated_features.route.proto:runtime_key",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this could be sorted alphabetically.

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 think we could locally, but tdb if we want to have these grouped by release rather than alphabetical overall. I'm going to merge without fixing just because I want it have a day or two where they're flipped and I'm around for questions and we can sort out ordering next release or in a follow-up?

@alyssawilk alyssawilk merged commit 19894ac into envoyproxy:master Apr 10, 2019
@alyssawilk alyssawilk deleted the deprecate branch May 7, 2019 19:11
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.

7 participants