docs: API review checklist (#14399)

* API review checklist

Signed-off-by: Mark D. Roth <roth@google.com>

Mirrored from https://github.com/envoyproxy/envoy @ b501569e116a3d17e93dd19f16b076ee513f90ea

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

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.