From 37037896379671084e4298e7531eaf846b757cab Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 12 Dec 2019 16:45:17 -0500 Subject: [PATCH] tooling: flags fixups (#9312) Changing runtime ASSERTs to ensure that all runtime guards are registered as true by default, or in the "special case" section (documented to be used with care, tracking issues etc.) Changing governance to note we should look at that section when we cut releases. Changing flags script to file code removal after guards have been true for 6 months. Risk Level: Meduim (if folks are using the upstream naming paradigm they'll fail asserts) Testing: existing tests Docs Changes: governance update Release Notes: n/a Fixes #8992 Signed-off-by: Alyssa Wilk --- GOVERNANCE.md | 2 + source/common/runtime/runtime_features.cc | 18 +++++ source/common/runtime/runtime_features.h | 4 ++ source/common/runtime/runtime_impl.cc | 3 +- tools/deprecate_version/deprecate_version.py | 76 ++++++++++---------- 5 files changed, 62 insertions(+), 41 deletions(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 6b7fb0e16a58..d782d34a16b5 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -106,6 +106,8 @@ or you can subscribe to the iCal feed [here](https://app.opsgenie.com/webcal/get * 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. +* Check source/common/runtime/runtime_features.cc and see if any runtime guards in + disabled_runtime_features should be reassessed, and ping on the relevant issues. ## When does a maintainer lose maintainer status diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 483f4682e3cc..73af6426f37a 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -33,6 +33,21 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.strict_authority_validation", }; +// This is a section for officially sanctioned runtime features which are too +// high risk to be enabled by default. Examples where we have opted to land +// features without enabling by default are swapping the underlying buffer +// implementation or the HTTP/1.1 codec implementation. Most features should be +// enabled by default. +// +// When features are added here, there should be a tracking bug assigned to the +// code owner to flip the default after sufficient testing. +constexpr const char* disabled_runtime_features[] = { + // Sentinel and test flag. + "envoy.reloadable_features.test_feature_false", + // Should be removed as part of https://github.com/envoyproxy/envoy/issues/8993 + "envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging", +}; + // This is a list of configuration fields which are disallowed by default in Envoy // // By default, use of proto fields marked as deprecated in their api/.../*.proto file will result @@ -70,6 +85,9 @@ RuntimeFeatures::RuntimeFeatures() { for (auto& feature : runtime_features) { enabled_features_.insert(feature); } + for (auto& feature : disabled_runtime_features) { + disabled_features_.insert(feature); + } } } // namespace Runtime diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index 2f7cce929f68..763d4d3d1c5c 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -29,12 +29,16 @@ class RuntimeFeatures { bool enabledByDefault(absl::string_view feature) const { return enabled_features_.find(feature) != enabled_features_.end(); } + bool existsButDisabled(absl::string_view feature) const { + return disabled_features_.find(feature) != disabled_features_.end(); + } private: friend class RuntimeFeaturesPeer; absl::flat_hash_set disallowed_features_; absl::flat_hash_set enabled_features_; + absl::flat_hash_set disabled_features_; }; using RuntimeFeaturesDefaults = ConstSingleton; diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 6ce1638ca4fb..cfc5cc3ae4ce 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -39,7 +39,8 @@ bool isLegacyFeature(absl::string_view feature) { } bool isRuntimeFeature(absl::string_view feature) { - return absl::StartsWith(feature, "envoy.reloadable_features."); + return RuntimeFeaturesDefaults::get().enabledByDefault(feature) || + RuntimeFeaturesDefaults::get().existsButDisabled(feature); } } // namespace diff --git a/tools/deprecate_version/deprecate_version.py b/tools/deprecate_version/deprecate_version.py index abe762d3ab5a..e35edd2db009 100644 --- a/tools/deprecate_version/deprecate_version.py +++ b/tools/deprecate_version/deprecate_version.py @@ -22,6 +22,8 @@ from __future__ import print_function +import datetime +from datetime import date from collections import defaultdict import os import re @@ -127,62 +129,56 @@ def CreateIssues(access_token, runtime_and_pr): raise -def GetRuntimeAlreadyTrue(): - """Returns a list of runtime flags already defaulted to true - """ - runtime_already_true = [] - runtime_features = re.compile(r'.*"(envoy.reloadable_features..*)",.*') - with open('source/common/runtime/runtime_features.cc', 'r') as features: - for line in features.readlines(): - match = runtime_features.match(line) - if match and 'test_feature_true' not in match.group(1): - print("Found existing flag " + match.group(1)) - runtime_already_true.append(match.group(1)) - - return runtime_already_true - - def GetRuntimeAndPr(): """Returns a list of tuples of [runtime features to deprecate, PR the feature was added] """ repo = Repo(os.getcwd()) - runtime_already_true = GetRuntimeAlreadyTrue() - # grep source code looking for reloadable features which are true to find the # PR they were added. - grep_output = subprocess.check_output('grep -r "envoy.reloadable_features\." source/', shell=True) features_to_flip = [] - runtime_feature_regex = re.compile(r'.*(source.*cc).*"(envoy.reloadable_features\.[^"]+)".*') - for line in grep_output.splitlines(): - match = runtime_feature_regex.match(str(line)) - if match: - filename = (match.group(1)) - runtime_guard = match.group(2) - # If this runtime guard isn't true, ignore it for this release. - if not runtime_guard in runtime_already_true: - continue - # For true runtime guards, walk the blame of the file they were added to, - # to find the pr the feature was added. - for commit, lines in repo.blame('HEAD', filename): - for line in lines: - if runtime_guard in line: - pr = (int(re.search('\(#(\d+)\)', commit.message).group(1))) - # Add the runtime guard and PR to the list to file issues about. - features_to_flip.append((runtime_guard, pr)) - # Make sure if grep finds multiple spots we only do the work one time. - runtime_already_true.remove(runtime_guard) + runtime_features = re.compile(r'.*"(envoy.reloadable_features..*)",.*') - else: - print('no match in ' + str(line) + ' please address manually!') + removal_date = date.today() - datetime.timedelta(days=183) + found_test_feature_true = False - return features_to_flip + # Walk the blame of runtime_features and look for true runtime features older than 6 months. + for commit, lines in repo.blame('HEAD', 'source/common/runtime/runtime_features.cc'): + for line in lines: + match = runtime_features.match(line) + if match: + runtime_guard = match.group(1) + if runtime_guard == 'envoy.reloadable_features.test_feature_false': + print("Found end sentinel\n") + if not found_test_feature_true: + # The script depends on the cc file having the true runtime block + # before the false runtime block. Fail if one isn't found. + print('Failed to find test_feature_true. Script needs fixing') + sys.exit(1) + return features_to_flip + if runtime_guard == 'envoy.reloadable_features.test_feature_true': + found_test_feature_true = True + continue + pr = (int(re.search('\(#(\d+)\)', commit.message).group(1))) + pr_date = date.fromtimestamp(commit.committed_date) + removable = (pr_date < removal_date) + # Add the runtime guard and PR to the list to file issues about. + print('Flag ' + runtime_guard + ' added at ' + str(pr_date) + ' ' + + (removable and 'and is safe to remove' or 'is not ready to remove')) + if removable: + features_to_flip.append((runtime_guard, pr)) + print('Failed to find test_feature_false. Script needs fixing') + sys.exit(1) if __name__ == '__main__': runtime_and_pr = GetRuntimeAndPr() + if not runtime_and_pr: + print('No code is deprecated.') + sys.exit(0) + access_token = os.getenv('GH_ACCESS_TOKEN') if not access_token: print(