From b5f7974c3e92a6de68e2e6f48cc22ab97f060ee7 Mon Sep 17 00:00:00 2001 From: "data-plane-api(Azure Pipelines)" Date: Wed, 27 Apr 2022 04:14:27 +0000 Subject: [PATCH] Update STYLE.md with instructions that actually work (#20808) While trying to send a pull request for an extension I was baffled multiple times by poor error messages and surprising behaviors. The alterations here each derive from a confusing thing that mostly also didn't get any clear "someone understood it better" signs from the dev slack channel. First surprise, if you use option (udpa.annotations.file_status).work_in_progress = true; like the doc suggested, proto_format.sh would remove it. Suggestion from the slack channel was to use xds.annotations.v3.file_status instead, which this updates STYLE.md to reflect. (related slack thread) Added a link to security posture and status documentation, which was otherwise hard to make sense of without foreknowledge. Adjusted the section that suggests using work_in_progress=true or package_version_status=ACTIVE - any combination of these (with either xds or udpa version of work_in_progress) that doesn't involve package_version_status=ACTIVE causes proto_format.sh to delete the file entirely. So the only viable options appear to be just package_version_status=ACTIVE, or that and xds work_in_progress=true which is already covered in an earlier bulletpoint. Clarified that without ACTIVE, proto_format.sh will delete the file (which hopefully will make it more discoverable for developers experiencing that symptom). Added a mention of api/BUILD's v3_protos section, without which proto_format.sh will automatically remove any imports of your new proto from other protos. Added mention of [#extension:] tag, which is also necessary, and extra detail about where [#extension-category:] has to go (it doesn't appear to work if present at the top-level). (related slack thread) Risk Level: None, documentation only. Testing: Verified that the old documentation doesn't work for at least one developer, and the new documentation is better. :) Docs Changes: That's all this is. Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Raven Black Mirrored from https://github.com/envoyproxy/envoy @ 2014cbea0f67e9513137eb6b4be3cb92ba437244 --- STYLE.md | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/STYLE.md b/STYLE.md index 35463008..bb55cbb2 100644 --- a/STYLE.md +++ b/STYLE.md @@ -116,8 +116,8 @@ organization](#package-organization) above. To add an extension config to the API, the steps below should be followed: 1. If this is still WiP and subject to breaking changes, please tag it - `option (udpa.annotations.file_status).work_in_progress = true;` and - optionally hide it from the docs (`[#not-implemented-hide:]`. + `option (xds.annotations.v3.file_status).work_in_progress = true;` and + optionally hide it from the docs (`[#not-implemented-hide:]`). 1. Place the v3 extension configuration `.proto` in `api/envoy/extensions`, e.g. `api/envoy/extensions/filters/http/foobar/v3/foobar.proto` together with an initial BUILD file: ```bazel @@ -130,9 +130,24 @@ To add an extension config to the API, the steps below should be followed: ) ``` 1. Update [source/extensions/extensions_metadata.yaml](../source/extensions/extensions_metadata.yaml) - with the category, security posture, and status. The category field will have to match an - annotation of the form `// [#extension-category: your.extension.category]` - in one of the proto files for the docs build to pass. + with the category, [security posture, and status](../EXTENSION_POLICY.md#extension-stability-and-security-posture). + * Any extension category added to `extensions_metadata.yaml` should be annotated in precisely one proto file, associated with a field of a proto message. e.g. + ```proto + message SomeMessage { + // An ordered list of http filters + // [#extension-category: envoy.http.filters] + repeated core.v3.TypedExtensionConfig http_filter_extensions = 1; + } + ``` + * Each extension added to `extensions_metadata.yaml` should have precisely one proto file annotated with the extension name. e.g. + ```proto + // [#protodoc-title: Your New Filter] + // [#extension: envoy.http.filters.your_new_filter] + + // YourFilterConfig is the configuration for a YourFilter (write real documentation here). + message YourFilterConfig { + } + ``` 1. Update [source/extensions/extensions_build_config.bzl](../source/extensions/extensions_build_config.bzl) to include the new extension. @@ -141,12 +156,10 @@ To add an extension config to the API, the steps below should be followed: (and to not break the docs build). See the [key-value-store PR](https://github.com/envoyproxy/envoy/pull/17745/files) for an example of adding a new extension point to common. 1. Make sure your proto imports the v3 extension config proto (`import "udpa/annotations/status.proto";`) -1. Make sure your proto is either tracked as a work in progress - (`option (udpa.annotations.file_status).work_in_progress = true;`) - or ready to be used +1. Make sure your proto is tracked as ready to be used (`option (udpa.annotations.file_status).package_version_status = ACTIVE;`). - This is required to automatically include the config proto in [api/versioning/BUILD](versioning/BUILD). -1. Add a reference to the v3 extension config in (1) in [api/versioning/BUILD](versioning/BUILD) under `active_protos`. + This is required to automatically include the config proto in [api/versioning/BUILD](versioning/BUILD) under `active_protos`. Without it `proto_format.sh` will delete the proto file. +1. Add a reference to the v3 extension config in (2) in [api/BUILD](BUILD) under `v3_protos`. Without this, `proto_format.sh` will erase any imports of your proto file from other proto files. 1. If you introduce a new extension category, you'll also need to add its name under `EXTENSION_CATEGORIES` in: [tools/extensions/extensions_check.py](../tools/extensions/extensions_check.py). 1. Run `./tools/proto_format/proto_format.sh fix`. Before running the script, you will need to commit your local changes to make them effective, @@ -189,7 +202,7 @@ metadata. We describe these annotations below by category. * `option (udpa.annotations.file_migrate).move_to_package = "";` to denote that in the next major version of the API, the file will be moved to the given package. This is consumed by `protoxform`. -* `option (udpa.annotations.file_status).work_in_progress = true;` to denote a +* `option (xds.annotations.v3.file_status).work_in_progress = true;` to denote a file that is still work-in-progress and subject to breaking changes. ## Principles