Add file server filter#43243
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/43243/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
…yproxy#43265 Signed-off-by: Raven Black <ravenblack@dropbox.com>
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for this great contribution. Some initial comments to API.
|
/wait |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/retest |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/lgtm deps |
Yeah. I can also sponsor this new extension and @ravenblackx could add me to the codeowner list. |
wbpcode
left a comment
There was a problem hiding this comment.
LGTM overall except some comments. The file_streamer is a little hard to follow but I also didn't get a better way.
/wait
| // [#not-implemented-hide:] Directory operations currently have no async implementation. | ||
| message List { | ||
| } |
There was a problem hiding this comment.
nit: as a fresh new extension, I think any field that not implemented could be removed first because:
- We may find no one want it in the future.
- We may have a better API design in the future.
There was a problem hiding this comment.
We want it - the catch is just that it requires implementing async directory operations first which is going to be additional PRs that I didn't want to roll up into this already large PR.
And I wanted the API review before going for that implementation. :)
| createRouteSpecificFilterConfigTyped(const ProtoFileServerConfig& config, | ||
| Server::Configuration::ServerFactoryContext& context, | ||
| ProtobufMessage::ValidationVisitor& validator) override; |
There was a problem hiding this comment.
nit: it would be great if we can also add the createFilterFactoryFromProtoWithServerContext support here.
There was a problem hiding this comment.
I don't understand what that function is for. How does it differ from createFilterFactoryFromProtoTyped? It has the same arguments except for dropping DualInfo, and it loses the StatusOr error path of the return value, so I assume it has to use exceptions instead.
This comment
// This method is for dual filter to create filter from server context when it is configured
// in downstream. It won't be called if a dual filter is in upstream.
is not enlightening to me at all, as that seems to describe half of what createFilterFactoryFromProtoTyped is already doing. I don't understand why the default behavior is to throw an exception rather than just calling the other function that has to be overridden (or why it exists at all, given the name implies it exists to be the same as the other function but with ServerContext, but the other function already takes the same arguments).
There was a problem hiding this comment.
createFilterFactoryFromProtoWithServerContext makes us could configure a filter in the route rather then downstream or upstream filter chain. At the route, we only have the server context and even have no DualInfo.
All these are historical technical debt. If even you, a maintainer, are confused about this, I think we do need a completely refactor to these methods.
There was a problem hiding this comment.
Agree. I also have seen many configs using the DualInfo version, but haven't seen any actually consuming the DualInfo. So I would think a good option would be to have a default for each of the functions, that just calls createFilterFactoryFromProtoWithServerContext (the lowest common denominator of available arguments), and then only override that one for the common case.
Except that createFilterFactoryFromProtoWithServerContext doesn't return a status. :(
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/retest |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
Found what the fuzz test problem is, it's nothing to do with my filter, it's that the fuzz tester picks a "random" filter if there's no config, and adding one more filter changes the outcome of that randomness to a filter that crashes. |
|
#43627 to address the caused-but-unrelated test breakage. |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Commit Message: Add file server filter Additional Description: Filter facilitates asynchronous serving from files. Simplistic configuration for the common use-case. Further features (e.g. exclusions) can be added later as needed. Secured against path-escapes by using `std::filesystem::path` to verify that the path requested isn't altered by normalization, and that it's within a path explicitly configured to be shared. Risk Level: Negligible, as no change to existing operations, it's a new filter. It's marked work in progress and not assured secure, so should not be adopted at this time without due caution. Testing: I think there's 100% coverage, but CI isn't running right now. Docs Changes: New docs for it. Release Notes: Will add at the end, to minimize annoying merge conflicts. Platform Specific Features: Windows not supported, as AsyncFile operations don't currently support Windows. --------- Signed-off-by: Raven Black <ravenblack@dropbox.com> Signed-off-by: bjmask <11672696+bjmask@users.noreply.github.com>
Commit Message: Add file server filter Additional Description: Filter facilitates asynchronous serving from files. Simplistic configuration for the common use-case. Further features (e.g. exclusions) can be added later as needed. Secured against path-escapes by using `std::filesystem::path` to verify that the path requested isn't altered by normalization, and that it's within a path explicitly configured to be shared. Risk Level: Negligible, as no change to existing operations, it's a new filter. It's marked work in progress and not assured secure, so should not be adopted at this time without due caution. Testing: I think there's 100% coverage, but CI isn't running right now. Docs Changes: New docs for it. Release Notes: Will add at the end, to minimize annoying merge conflicts. Platform Specific Features: Windows not supported, as AsyncFile operations don't currently support Windows. --------- Signed-off-by: Raven Black <ravenblack@dropbox.com>
Commit Message: Add file server filter Additional Description: Filter facilitates asynchronous serving from files. Simplistic configuration for the common use-case. Further features (e.g. exclusions) can be added later as needed. Secured against path-escapes by using `std::filesystem::path` to verify that the path requested isn't altered by normalization, and that it's within a path explicitly configured to be shared. Risk Level: Negligible, as no change to existing operations, it's a new filter. It's marked work in progress and not assured secure, so should not be adopted at this time without due caution. Testing: I think there's 100% coverage, but CI isn't running right now. Docs Changes: New docs for it. Release Notes: Will add at the end, to minimize annoying merge conflicts. Platform Specific Features: Windows not supported, as AsyncFile operations don't currently support Windows. --------- Signed-off-by: Raven Black <ravenblack@dropbox.com>
Commit Message: Add file server filter Additional Description: Filter facilitates asynchronous serving from files. Simplistic configuration for the common use-case. Further features (e.g. exclusions) can be added later as needed. Secured against path-escapes by using `std::filesystem::path` to verify that the path requested isn't altered by normalization, and that it's within a path explicitly configured to be shared. Risk Level: Negligible, as no change to existing operations, it's a new filter. It's marked work in progress and not assured secure, so should not be adopted at this time without due caution. Testing: I think there's 100% coverage, but CI isn't running right now. Docs Changes: New docs for it. Release Notes: Will add at the end, to minimize annoying merge conflicts. Platform Specific Features: Windows not supported, as AsyncFile operations don't currently support Windows. --------- Signed-off-by: Raven Black <ravenblack@dropbox.com>
Commit Message: Add file server filter
Additional Description: Filter facilitates asynchronous serving from files. Simplistic configuration for the common use-case. Further features (e.g. exclusions) can be added later as needed. Secured against path-escapes by using
std::filesystem::pathto verify that the path requested isn't altered by normalization, and that it's within a path explicitly configured to be shared.Risk Level: Negligible, as no change to existing operations, it's a new filter. It's marked work in progress and not assured secure, so should not be adopted at this time without due caution.
Testing: I think there's 100% coverage, but CI isn't running right now.
Docs Changes: New docs for it.
Release Notes: Will add at the end, to minimize annoying merge conflicts.
Platform Specific Features: Windows not supported, as AsyncFile operations don't currently support Windows.