Skip to content

release: flipping deprecated features to be fatal-by-default #7549

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.

Sign up for GitHub

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

Merged
mattklein123 merged 2 commits into from
Jul 16, 2019
Merged

release: flipping deprecated features to be fatal-by-default #7549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e988d1d
release: flipping deprecated features to be fatal-by-default
alyssawilk Jul 11, 2019
8d942c9
canonical config
alyssawilk Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion GOVERNANCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ or you can subscribe to the iCal feed [here](https://app.opsgenie.com/webcal/get
following version. E.g., "1.7.0 (pending)".
* Run the deprecate_versions.py script (e.g. `sh tools/deprecate_version/deprecate_version.sh 1.8.0 1.10.0`)
to file tracking issues for code which can be removed.
* Run the deprecate_features.py script (e.g. `sh tools/deprecate_version/deprecate_features.sh`)
* Run the deprecate_features.py script (e.g. `sh tools/deprecate_features/deprecate_features.sh`)
to make the last release's deprecated features fatal-by-default. Submit the resultant PR and send
an email to envoy-announce.

Expand Down
2 changes: 1 addition & 1 deletion api/envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ message Cluster {
// **This field is deprecated**. Set the
// :ref:`load_assignment<envoy_api_field_Cluster.load_assignment>` field instead.
//
repeated core.Address hosts = 7 [deprecated = true];
repeated core.Address hosts = 7;

// Setting this is required for specifying members of
// :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`,
Expand Down
17 changes: 13 additions & 4 deletions configs/envoy_double_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,19 @@ tracing:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
access_token_file: "/etc/envoy/lightstep_access_token"
collector_cluster: lightstep_saas
runtime:
symlink_root: "/srv/runtime_data/current"
subdirectory: envoy
override_subdirectory: envoy_override
layered_runtime:
Copy link
Member

@mattklein123 mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

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

The equivalent of what we had before would be:

layered_runtime:
  layers:
    - name: root
      disk_layer:
        symlink_root: /srv/configset/envoydata/current
        subdirectory: envoy
    - name: override
      disk_layer:
        symlink_root: /srv/configset/envoydata/current
        subdirectory: envoy_override
        append_service_cluster: true
    - name: admin
      admin_layer: {}

Should we keep it equivalent?

layers:
- name: root
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy
- name: override
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy_override
append_service_cluster: true
- name: admin
admin_layer: {}
admin:
access_log_path: "/var/log/envoy/admin_access.log"
address:
Expand Down
17 changes: 13 additions & 4 deletions configs/envoy_front_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,19 @@ tracing:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
collector_cluster: lightstep_saas
access_token_file: "/etc/envoy/lightstep_access_token"
runtime:
symlink_root: /srv/runtime_data/current
subdirectory: envoy
override_subdirectory: envoy_override
layered_runtime:
layers:
- name: root
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy
- name: override
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy_override
append_service_cluster: true
- name: admin
admin_layer: {}
admin:
access_log_path: /var/log/envoy/admin_access.log
address:
Expand Down
17 changes: 13 additions & 4 deletions configs/envoy_service_to_service_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,10 +543,19 @@ tracing:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
access_token_file: "/etc/envoy/lightstep_access_token"
collector_cluster: lightstep_saas
runtime:
symlink_root: "/srv/runtime_data/current"
subdirectory: envoy
override_subdirectory: envoy_override
layered_runtime:
layers:
- name: root
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy
- name: override
disk_layer:
symlink_root: /srv/configset/envoydata/current
subdirectory: envoy_override
append_service_cluster: true
- name: admin
admin_layer: {}
admin:
access_log_path: /var/log/envoy/admin_access.log
address:
Expand Down
6 changes: 6 additions & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ constexpr const char* disallowed_features[] = {
// Acts as both a test entry for deprecated.proto and a marker for the Envoy
// deprecation scripts.
"envoy.deprecated_features.deprecated.proto:is_deprecated_fatal",
"envoy.deprecated_features.bootstrap.proto:runtime",
Copy link
Member

@mattklein123 mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

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

nit: I would probably alpha sort all of these if possible.

Copy link
Member

@mattklein123 mattklein123 Jul 16, 2019

Choose a reason for hiding this comment

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

I will alpha order when I do my follow up PR.

"envoy.deprecated_features.redis_proxy.proto:catch_all_cluster",
"envoy.deprecated_features.lds.proto:use_original_dst",
"envoy.deprecated_features.server_info.proto:max_stats",
Copy link
Member

@mattklein123 mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

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

I think this and below are on output protos so they won't have any effect. Not sure how we could know this. Thoughts?

Copy link
Contributor Author

@alyssawilk alyssawilk Jul 15, 2019

Choose a reason for hiding this comment

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

Good point. No ideas here - I'd be inclined to leave them in to avoid script churn.
Maybe have file or proto naming in v3 to make things more clear going forward?
@htuch

"envoy.deprecated_features.redis_proxy.proto:cluster",
"envoy.deprecated_features.server_info.proto:max_obj_name_len",
Copy link
Member

@mattklein123 mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

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

I think we need

envoy/source/common/config/utility.cc

Line 302 in b3f848d

"envoy.deprecated_features.v1_filter_json_config")) {
also?

Copy link
Contributor Author

@alyssawilk alyssawilk Jul 15, 2019

Choose a reason for hiding this comment

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

Oh man, I totally missed this in review but this should have been envoy.reloadable_features.v1_filter_json_config which is why it wasn't caught.

I tried flipping it, and it broke the bootstrap test pretty hard. Beyond needing the same runtime layers update, there's some issue with proto constraint which I've tracked down to a really confusing red herring where when we to do proto to json conversion we end up failing and so having a completely empty proto, so failing the first validator which checks for non-empty bytes

Given this didn't land as a config change what do we think of renaming, and flipping in its own PR, ideally after some digging into the hot restart test?

Copy link
Member

@mattklein123 mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

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

Sure whatever you think is best. I mainly want to get this deprecation moving since this will allow us to delete a ton of code (all of v1).

Copy link
Contributor Author

@alyssawilk alyssawilk Jul 15, 2019

Choose a reason for hiding this comment

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

Would you have cycles to look into what's going on with the filter config? I was going to ask we assign whoever introduced the variable to sorting out the test and looks like it was you :-P

Copy link
Member

@mattklein123 mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

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

Yes, sorry, what do you want me to do exactly? If you give me a diff that is broken I can debug it. :)

Copy link
Contributor Author

@alyssawilk alyssawilk Jul 15, 2019

Choose a reason for hiding this comment

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

I think we land this as-is, and you flip the v1 json separately, since it currently doesn't pass tests?

Copy link
Member

@mattklein123 mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

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

OK got it, but you want me to change the name also?

"envoy.deprecated_features.config_source.proto:UNSUPPORTED_REST_LEGACY",
"envoy.deprecated_features.ext_authz.proto:use_alpha",
"envoy.deprecated_features.route.proto:enabled",
Expand Down