docs: document type URLs for extensions#21707
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
Docs for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/21707/docs/index.html The docs are (re-)rendered each time the CI |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
@phlax Could you help me to fix the extensions schema linter error? I can't find the code that executes it. |
|
yep - the error messages are not great here (i raised a bug upstream to try and improve) the "shape" it is expecting - is here which translates to the spec here https://github.com/envoyproxy/pytooling/blob/603f4f2d0b53aabc41c5a6b59ea961131ee5f754/envoy.code.check/envoy/code/check/typing.py#L22 let me check the change... |
|
we need to add |
|
Thanks, @phlax No, type_urls is not required, some extensions are still un-typed, but most are. There are gaps also in the manifest, but now it's all easy to see in one document. |
|
cool for non required then it wants to be added here https://github.com/envoyproxy/pytooling/blob/main/envoy.code.check/envoy/code/check/typing.py#L18 do you want me to do it - happy to review if you want to |
|
Sent envoyproxy/toolshed#574. |
|
A suggestion, maybe we can show category, qualified name and type URL in a single tips, or at least qualified name and type URL? |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
@zhxie combined categories and type URLs together. |
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for validating the extensions yaml file!
Left small comment.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
There's a bunch of missing extensions that will need follow-up:
We can follow-up with the clean-up after this PR. |
adisuissa
left a comment
There was a problem hiding this comment.
LGTM, thanks.
I think this PR should have another pair of eyes assessing the impact of the extensions "names" and the "envoy.access_logger.extensions_filters" category name modifications.
@mattklein123 can you review this or suggest someone?
|
@kyessenov can you provide a more detailed summary of what this change does? I'm not really sure what the end user impact is. Is this going to break anyone's existing configuration? Thank you. /wait-any |
|
@mattklein123 Posted updated description. This change is mostly docs- and build-related, except for the new extension |
phlax
left a comment
There was a problem hiding this comment.
this is definitely the missing piece in the api/config docs so in general i would be inclined to land and iterate
that said, i still think we need to clarify the relationship between extension qualified names and type urls - their necessity/specificity etc
the other thing im a little confused by (i think you may have answered this elswhere, and i guess may related to use or otherwise of typed_config) some of the extensions only have qualified names
| {% endfor %} | ||
|
|
||
| {% if type_urls|length > 0 %} | ||
| This extension can be configured with the following type URLs: |
There was a problem hiding this comment.
my main concern here is that its not clear what this means - either if you are coming from a position of zero-knowledge, or trying to discriminate/associate/understand type_urls and extension name
in most cases at least, would it not be more correct to say that this extensions must be configured with this type URL ?
There was a problem hiding this comment.
Good point, changing to must.
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. A few comments/questions. Very happy to see the docs improved in this area.
/wait
| // [#protodoc-title: SimpleHttpCache CacheFilter storage plugin] | ||
|
|
||
| // [#extension: envoy.cache.simple_http_cache] | ||
| // [#extension: envoy.extensions.http.cache.simple] |
There was a problem hiding this comment.
Is this right? Per below shouldn't it be envoy.http.cache.simple ? Something seems off.
There was a problem hiding this comment.
Yes, it's right: https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc#L300
It's an extension within an extension so might be confusing.
| virtual bool disableFactory(absl::string_view) PURE; | ||
| virtual bool isFactoryDisabled(absl::string_view) const PURE; | ||
| virtual absl::flat_hash_map<std::string, std::vector<std::string>> registeredTypes() const PURE; | ||
| virtual absl::string_view canonicalFactoryName(absl::string_view) const PURE; |
There was a problem hiding this comment.
This function is used deprecated filter name to get canonical filter name. For example, use envoy.router to get envoy.filters.http.router. So I think it not relates to #21525 directly.
| Runtime::Loader& runtime, Random::RandomGenerator& random) PURE; | ||
|
|
||
| std::string category() const override { return "envoy.access_logger.extension_filters"; } | ||
| std::string category() const override { return "envoy.access_loggers.extension_filters"; } |
There was a problem hiding this comment.
This is potentially a breaking change? Is this the only one? What are the implications? Can we avoid breaking people and if not we should at the very least have a release note?
There was a problem hiding this comment.
Adding release notes. This was added recently by @douglas-reid so I'll make sure we don't break us. I couldn't think of a reason why category name is significant for users.
| # CacheFilter plugins | ||
| # | ||
| "envoy.cache.simple_http_cache": "//source/extensions/filters/http/cache/simple_http_cache:config", | ||
| "envoy.extensions.http.cache.simple": "//source/extensions/filters/http/cache/simple_http_cache:config", |
There was a problem hiding this comment.
Same question about "extensions"
There was a problem hiding this comment.
This aligns to what is in the code as well.
| {% endfor %} | ||
|
|
||
| {% if type_urls|length > 0 %} | ||
| This extension can be configured with the following type URLs: |
|
@phlax Re: question about missing type URLs for some extensions. There are several reasons for that which I mentioned above. One is that they are just labelled differently internally in the code and in extension schema, so we need to fix that later on a case by case basis. Another is some extensions are purely internal and never need to be configured. |
|
|
||
| {% if type_urls|length > 0 %} | ||
| This extension can be configured with the following type URLs: | ||
| This extension must be configured with the following type URLs: |
There was a problem hiding this comment.
| This extension must be configured with the following type URLs: | |
| This extension must be configured with one of the following type URLs: |
not sure about this suggestion, and the pluralization is a bit wierd - but i think this could mean must be configured with all type urls
There was a problem hiding this comment.
Yes, updated once again. It's just one URL 90% cases, so we can maybe add two types of templates, for single one and plural separately.
mattklein123
left a comment
There was a problem hiding this comment.
LGTM to ship and iterate. Thank you so much! Unfortunately I think needs another main merge. Let's just do that and we can quickly get this in. Thank you.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
Follow-up to #21707 with a focus on back-filling more extension type URLs. Renames extensions_build_config and extensions_metadata to the names in the internal extension registry. For preserve_case, we deprecate the short name with the fully qualified name (both names are valid in the interim). Risk Level: medium, only preserve_case name changes Testing: regression Docs Changes: yes Release Notes: yes Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: document type URLs for extensions
Additional Description: add validation to extension metadata (and fix discovered issue), annotate with type URLs, and print type URLs on extension pages. The end-result is that every extension is now annotated with type URLs it accepts, making it easier to get started using them. This is accomplished by adding a new field
type_urlto the extension manifest. While doing so, I found several inconsistencies. There are three places where extension names and categories are used, and they don't always match:nameandcategory. These names should not be changed ideally, as the names are literal in configs, and it's possible for the old config to be rejected.#extension-category. I think these are fine to change as long as the docs are improved.extension.bzlfile. These are purely build artifacts, so should be fairly safe to change. The only side-effect is downstream repos will need to copy the changes.There are too many mis-matches to fix at once, so I only fixed few most visible:
envoy.access_loggers.streamto reflect the reality that there are two extensionsstdoutandstderr.envoy.access_loggers.extension_filtersper discussion with @adisuissa.envoy.extensions.http.cache.simple.Risk Level: medium
Testing: yes
Docs Changes: yes
Release Notes: none
Issue: #13167