From 5cd1b54fffd87a8c5c7452a5a6ae9f8f12da4898 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 14 Dec 2020 10:52:50 -0800 Subject: [PATCH 1/5] API review checklist Signed-off-by: Mark D. Roth --- api/review_checklist.md | 137 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 api/review_checklist.md diff --git a/api/review_checklist.md b/api/review_checklist.md new file mode 100644 index 0000000000000..0544871273657 --- /dev/null +++ b/api/review_checklist.md @@ -0,0 +1,137 @@ +# API Review Checklist + +This checklist is intended to be used when reviewing xDS API changes. +Users who wish to contribute API changes should read this and proactively +consider the answers to these questions before sending a PR. + +## Feature Enablement +- Are the default values going to cause behavior changes for existing users + who do not know about the change and have not updated the resources being + served by their control plane? + - If yes, do we have some estimate of how many users will be affected? + - Why is it justified to change the default behavior, rather than making + this feature opt-in? + - Some possible justifications include security concerns with existing + behavior, or a desire to eliminate legacy behavior. + - What is the plan to make this change in a safe way? For example, is the + transition going to be staged over the course of several minor xDS versions? + - How will we warn users about this change? + - Possible ways to do this include release notes, announcements, warnings + from the code, etc. +- Will users have a way to disable this change if it causes problems? + - If not, why do we think that's okay? (It might be the case that we think + it will not actually affect anyone, or no one will care.) + - If so, is the mechanism to disable it part of the xDS API, or is it + acceptable to have a separate knob for this in the client? (See also + "Genericness" below -- if this is not part of the API, will every xDS + client need to add a different knob? Is consistency across clients + important for this?) + +## Style +- If an enum is being introduced, should this be a oneof with empty messages + for future API growth? +- When a new field is introduced for a distinct purpose, should this be a + message to allow for future API growth? +- For time related fields, is a Duration or Timestamp WKT used rather than + raw integers for seconds etc? +- Is the PR aligned with the [API style guidelines](STYLE.md)? + +## Validation Rules +- Is the field mandatory? Does it have the required rule? +- Is the field numeric? Is it bounded correctly? +- Is the field repeated? Does it have the correct min/max items numbers? Are + the values unique? +- If a field may eventually be repeated but initially there is a desire to + cap it to a single value, consider using repeated with a max length of 1, + which is easier to relax in the future. + +## Deprecations +- When a field or enum value is deprecated, according to the minor/patch + versioning policy this implies a minor version for support removal. Is the + work necessary to add support for the newer replacement field acceptable to + known xDS clients in this time frame? +- No deprecations are allowed when an alternative is "not ready yet" in any + major known xDS client (Envoy or gRPC at this point). +- Is this deprecated field documented in a way that points to the preferred + newer method of configuration? + +## Extensibility +- Is this feature being directly added into the API itself, or is it being + introduced via an extension point? +- If not via an extension point, why not? +- If no appropriate extension point currently exists, is this a good + opportunity to add one? Can we move some existing "core" functionality + into a set of standardized plugins for an extension point? +- Do we have good documentation showing what to plug into the extension point? + (At minimum, it should have a comment pointing to the common protos to + be plugged into the extension point.) + +## Consistency +- Can the proposed API reuse part or all of other APIs? + - Can some other API be refactored to be part of it, or vice versa? + - Example: Can it use common types such as matcher or number? +- Are there similar structures that already exist? +- Is the naming convention consistent with other APIs? + +## Interactions With Other Features +- Will this feature interact in strange ways with other features, either + existing or planned? + - For example, if you are defining a new cluster type, how will the + new type implement all of the features currently configured via CDS? + - If this is a change in the upstream side of the API, will it work properly + with LRS load reporting? +- Will there be combinations of features that won't work properly? If so, + please document each combination that won't work and justify why this is + okay. Is there some other way to structure this feature that would not + cause conflicts with other features? + +## Genericness +- Is this an Envoy-specific or proxy-specific feature? How will it apply to xDS clients other than Envoy (e.g., gRPC)? + +## Dependencies +- Does this feature pull in any new dependencies for clients? +- Are these dependencies optional or required? +- Will the dependencies cause problems for any known xDS clients? + +## Failure Modes +- What is the failure mode if this feature is configured but is not working + for some reason? +- Is the failure mode what users will expect/want? +- Is this failure mode specified in the API, or is each client expected to + handle it on its own? Consistency across clients is preferred; if there's + a reason this isn't feasible, please explain. + +## Scalability +- Does this feature add new per-request functionality? How much overhead does + it add on the per-request path? +- Are there ways that the API could be structured differently to make it + possible to implement the feature in more efficient ways? (Even if this + efficiency is not needed now, it may be something we will need in the future, + and we will save pain in the long run if we structure the API in a way that + gives us the flexibility to change the implementation later.) + +## Monitoring +- Is there any behavior associated with this feature that will require + monitoring? +- How will the data be exposed to monitoring? +- Is the monitoring configuration part of the xDS API, or is it client-specific? + +## Documentation +- Can a user look at the docs and understand it without a bunch of extra + context? +- Pay special attention to documentation around extensions and `typed_config`. + Users generally find this extremely confusing. There should be examples + showing how to configure extension points and optimally all known public + extensions (there is tooling work in progress to automate this). +- Larger features should contain architecture overview documentation with + relevant cross-linking. +- Relevant differences between clients need to be documented (in the future + we will build tooling to allow for common documentation as well as per-client + documentation). + +## Where to Put New Protos +- The xDS API is currently partly in the Envoy repo and partly in the + cncf/xds repo. We will move pieces of the API to the latter repo + slowly over time, as they become less Envoy-specific, until eventually + the whole API has been moved there. If your change involves adding + new protos, they should generally go in the new repo. From f85cba764e0fe69eed40637bbc34282042e1a38b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 17 Dec 2020 08:52:00 -0800 Subject: [PATCH 2/5] code review comments Signed-off-by: Mark D. Roth --- CONTRIBUTING.md | 7 +++++++ PULL_REQUESTS.md | 8 ++++++++ PULL_REQUEST_TEMPLATE.md | 1 + api/STYLE.md | 6 ++++++ api/review_checklist.md | 10 ++++------ 5 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 91642701329d9..3c5bc71485611 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -233,6 +233,13 @@ and false. * If a PR includes a deprecation/breaking change, notification should be sent to the [envoy-announce](https://groups.google.com/forum/#!forum/envoy-announce) email list. +# API changes + +If you change anything in the [api tree](https://github.com/envoyproxy/envoy/tree/master/api), +please read the [API Review +Checklist](https://github.com/envoyproxy/envoy/tree/master/api/review_checklist.md) +and make sure that your changes have addressed all of the considerations listed there. + # Adding new extensions For developers adding a new extension, one can take an existing extension as the starting point. diff --git a/PULL_REQUESTS.md b/PULL_REQUESTS.md index 3fb17835ebd01..e8a5a390b0485 100644 --- a/PULL_REQUESTS.md +++ b/PULL_REQUESTS.md @@ -111,3 +111,11 @@ PR description. If you mark existing APIs or code as deprecated, when the next release is cut, the deprecation script will create and assign an issue to you for cleaning up the deprecated code path. + +### API Changes + +If this PR changes anything in the [api tree](https://github.com/envoyproxy/envoy/tree/master/api), +please read the [API Review +Checklist](https://github.com/envoyproxy/envoy/tree/master/api/review_checklist.md) +and make sure that your changes have addressed all of the considerations listed there. +Any relevant considerations should be documented under "API Considerations" in the PR description. diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md index 366209eed9299..ebc52d14eb34e 100644 --- a/PULL_REQUEST_TEMPLATE.md +++ b/PULL_REQUEST_TEMPLATE.md @@ -22,3 +22,4 @@ Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Deprecated:] +[Optional API Considerations:] diff --git a/api/STYLE.md b/api/STYLE.md index 7e15d2ec9f1b5..79a08ee67f79a 100644 --- a/api/STYLE.md +++ b/api/STYLE.md @@ -81,6 +81,12 @@ In addition, the following conventions should be followed: pattern forces developers to explicitly choose the correct enum value for their use case, and avoid misunderstanding of the default behavior. +* For time-related fields, prefer using the well-known types `google.protobuf.Duration` or + `google.protobuf.Timestamp` instead of raw integers for seconds. + +* If a field is going to contain raw bytes rather than a human-readable string, the field should + be of type `bytes` instead of `string`. + * Proto fields should be sorted logically, not by field number. ## Package organization diff --git a/api/review_checklist.md b/api/review_checklist.md index 0544871273657..13abbdce3fe0c 100644 --- a/api/review_checklist.md +++ b/api/review_checklist.md @@ -28,12 +28,6 @@ consider the answers to these questions before sending a PR. important for this?) ## Style -- If an enum is being introduced, should this be a oneof with empty messages - for future API growth? -- When a new field is introduced for a distinct purpose, should this be a - message to allow for future API growth? -- For time related fields, is a Duration or Timestamp WKT used rather than - raw integers for seconds etc? - Is the PR aligned with the [API style guidelines](STYLE.md)? ## Validation Rules @@ -65,6 +59,10 @@ consider the answers to these questions before sending a PR. - Do we have good documentation showing what to plug into the extension point? (At minimum, it should have a comment pointing to the common protos to be plugged into the extension point.) +- If an enum is being introduced, should this be a oneof with empty messages + for future API growth? +- When a new field is introduced for a distinct purpose, should this be a + message to allow for future API growth? ## Consistency - Can the proposed API reuse part or all of other APIs? From 6c90c344bd3242e8aff743e1b3179345f46340ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 17 Dec 2020 09:07:28 -0800 Subject: [PATCH 3/5] more code review comments Signed-off-by: Mark D. Roth --- api/review_checklist.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/review_checklist.md b/api/review_checklist.md index 13abbdce3fe0c..375262d6725bf 100644 --- a/api/review_checklist.md +++ b/api/review_checklist.md @@ -45,7 +45,9 @@ consider the answers to these questions before sending a PR. work necessary to add support for the newer replacement field acceptable to known xDS clients in this time frame? - No deprecations are allowed when an alternative is "not ready yet" in any - major known xDS client (Envoy or gRPC at this point). + major known xDS client (Envoy or gRPC at this point). If you are not sure + about the current state of a feature in the major known xDS clients, ask + one of the API shepherds. - Is this deprecated field documented in a way that points to the preferred newer method of configuration? From 8821b7787b82d9b15c89a137c8828f7dd65111f7 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 21 Dec 2020 10:47:04 -0800 Subject: [PATCH 4/5] code review changes Signed-off-by: Mark D. Roth --- PULL_REQUEST_TEMPLATE.md | 2 +- api/review_checklist.md | 29 +++++++++++++++++++++++------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md index ebc52d14eb34e..0cb0d0daad54b 100644 --- a/PULL_REQUEST_TEMPLATE.md +++ b/PULL_REQUEST_TEMPLATE.md @@ -22,4 +22,4 @@ Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Deprecated:] -[Optional API Considerations:] +[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/master/api/review_checklist.md):] diff --git a/api/review_checklist.md b/api/review_checklist.md index 375262d6725bf..f4fc0dbbd489a 100644 --- a/api/review_checklist.md +++ b/api/review_checklist.md @@ -31,6 +31,8 @@ consider the answers to these questions before sending a PR. - Is the PR aligned with the [API style guidelines](STYLE.md)? ## Validation Rules +- Does the field have protocgen-validate rules to enforce constraints on + its value? - Is the field mandatory? Does it have the required rule? - Is the field numeric? Is it bounded correctly? - Is the field repeated? Does it have the correct min/max items numbers? Are @@ -45,15 +47,16 @@ consider the answers to these questions before sending a PR. work necessary to add support for the newer replacement field acceptable to known xDS clients in this time frame? - No deprecations are allowed when an alternative is "not ready yet" in any - major known xDS client (Envoy or gRPC at this point). If you are not sure - about the current state of a feature in the major known xDS clients, ask - one of the API shepherds. + major known xDS client (Envoy or gRPC at this point), unless the + maintainers of that xDS client have signed off on the change. If you are not + sure about the current state of a feature in the major known xDS clients, + ask one of the API shepherds. - Is this deprecated field documented in a way that points to the preferred newer method of configuration? ## Extensibility - Is this feature being directly added into the API itself, or is it being - introduced via an extension point? + introduced via an extension point (i.e., using `TypedExtensionConfig`)? - If not via an extension point, why not? - If no appropriate extension point currently exists, is this a good opportunity to add one? Can we move some existing "core" functionality @@ -72,6 +75,11 @@ consider the answers to these questions before sending a PR. - Example: Can it use common types such as matcher or number? - Are there similar structures that already exist? - Is the naming convention consistent with other APIs? +- If there are new proto messages being added, are they in the right + place in the tree? Consider not just the current users of this proto + but also other possible uses in the future. Would it make more sense + to make the proto a generic type and put it in a common location, so + that it can be used in other places in the future? ## Interactions With Other Features - Will this feature interact in strange ways with other features, either @@ -84,14 +92,18 @@ consider the answers to these questions before sending a PR. please document each combination that won't work and justify why this is okay. Is there some other way to structure this feature that would not cause conflicts with other features? +- If this change involves extension configuration, how will it interact + with ECDS? ## Genericness -- Is this an Envoy-specific or proxy-specific feature? How will it apply to xDS clients other than Envoy (e.g., gRPC)? +- Is this an Envoy-specific or proxy-specific feature? How will it apply to + xDS clients other than Envoy (e.g., gRPC)? ## Dependencies - Does this feature pull in any new dependencies for clients? - Are these dependencies optional or required? -- Will the dependencies cause problems for any known xDS clients? +- Will the dependencies cause problems for any known xDS clients (e.g., + [Envoy's dependency policy](https://github.com/envoyproxy/envoy/blob/master/DEPENDENCY_POLICY.md))? ## Failure Modes - What is the failure mode if this feature is configured but is not working @@ -109,6 +121,11 @@ consider the answers to these questions before sending a PR. efficiency is not needed now, it may be something we will need in the future, and we will save pain in the long run if we structure the API in a way that gives us the flexibility to change the implementation later.) +- How does this feature affect config size? For example, instead of + adding a huge mandatory proto to every single route entry, consider + ways of setting it at the virtual host level and then overriding only + the parts that change on a per-route basis. +- Will the change require multiple round trips via the REST API? ## Monitoring - Is there any behavior associated with this feature that will require From c4ceee77ae458aa016e93bfeb479655ffb0644f9 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 19 Jan 2021 12:32:18 -0800 Subject: [PATCH 5/5] fix_format Signed-off-by: Mark D. Roth --- api/review_checklist.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/review_checklist.md b/api/review_checklist.md index f4fc0dbbd489a..bce9f188dad23 100644 --- a/api/review_checklist.md +++ b/api/review_checklist.md @@ -90,7 +90,7 @@ consider the answers to these questions before sending a PR. with LRS load reporting? - Will there be combinations of features that won't work properly? If so, please document each combination that won't work and justify why this is - okay. Is there some other way to structure this feature that would not + okay. Is there some other way to structure this feature that would not cause conflicts with other features? - If this change involves extension configuration, how will it interact with ECDS? @@ -148,7 +148,7 @@ consider the answers to these questions before sending a PR. ## Where to Put New Protos - The xDS API is currently partly in the Envoy repo and partly in the - cncf/xds repo. We will move pieces of the API to the latter repo + cncf/xds repo. We will move pieces of the API to the latter repo slowly over time, as they become less Envoy-specific, until eventually - the whole API has been moved there. If your change involves adding + the whole API has been moved there. If your change involves adding new protos, they should generally go in the new repo.