docs: API review checklist (#14399)

* API review checklist

Signed-off-by: Mark D. Roth <[email protected]>

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.

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.
+
+### <a name="api"></a>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.

PULL_REQUEST_TEMPLATE.md

@@ -22,3 +22,4 @@ Platform Specific Features:
 [Optional Runtime guard:]
 [Optional Fixes #Issue]
 [Optional Deprecated:]
+[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/master/api/review_checklist.md):]

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

api/review_checklist.md

@@ -0,0 +1,154 @@
+# 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
+- 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
+  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), 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 (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
+  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.)
+- 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?
+  - 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?
+- 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
+  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?
+- 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)?
+
+## 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 (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
+  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.)
+- 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
+  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.