Conversation
Signed-off-by: Ashley Hedberg <ahedberg@google.com>
Signed-off-by: Ashley Hedberg <ahedberg@google.com>
Signed-off-by: Ashley Hedberg <ahedberg@google.com>
|
/assign @htuch |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Thanks, looks good, one design question, the rest of the comments are mechanical.
/wait
| * Context passed to filters to access resources from non-Envoy parts of the | ||
| * process. | ||
| */ | ||
| class ProcessContext { |
There was a problem hiding this comment.
If we're only ever going to support get(), I guess it might be possible to eliminate ProcessObject and just pass in a subclass of ProcessContext with the bits you are after. That said, I think it's good to have this indirection as we can grow it over time.
One consideration I'm interested in your thoughts on. Let's say we have two filters and they want to get at different aspects of the state. Would it make sense to have a map from a string to the ProcessObjects, or have them aware of the internals of ProcessObject to do this? I'm thinking the former would make sense when you have multiple parties trying to collaborate on ProcessObject, the latter when it's single party.
There was a problem hiding this comment.
I had envisioned a map from string to ProcessObjects inside ProcessContext at some point, and then get() would do the lookup by that name. I didn't implement it here because we don't need it for our use case, and I wasn't sure if other folks would. I certainly could if we want to go with that design from the beginning, though.
There was a problem hiding this comment.
I think it's fine as is, the thing to be aware of then is in the future we might see an API breaking change. Let's ship and iterate.
Signed-off-by: Ashley Hedberg <ahedberg@google.com>
Signed-off-by: Ashley Hedberg <ahedberg@google.com>
|
/retest |
|
🔨 rebuilding |
| * Context passed to filters to access resources from non-Envoy parts of the | ||
| * process. | ||
| */ | ||
| class ProcessContext { |
There was a problem hiding this comment.
I think it's fine as is, the thing to be aware of then is in the future we might see an API breaking change. Let's ship and iterate.
* master: test: Add coverage for IsolatedStoreImpl::find* (envoyproxy#7043) server: add ProcessContext (envoyproxy#7018) config: Implement both versions of onConfigUpdate() everywhere (envoyproxy#6879) gzip: add test for various compression strategy and level (envoyproxy#7055) Fix typo in comment for rds.RouteConfiguration.validate_clusters (envoyproxy#7056) mysql_filter: add handling for partial messages (envoyproxy#6885) migrate from v2alpha to v2 (envoyproxy#7044) tests: fix tsan test flake (envoyproxy#7052) upstream: fix HostUtility::healthFlagsToString (envoyproxy#7051) tech debt: eliminate absl::make_unique (envoyproxy#7034) router: add a route name field in route.Route list (envoyproxy#6776) ext_authz: configurable HTTP status code for network errors. (envoyproxy#6669) stats: remove const-cast for symbol-table in edcs_filter_test.cc (envoyproxy#7045) build: bump libevent to 3b1864b. (envoyproxy#7012) stats: improve test-coverage for a few stats-related functions. (envoyproxy#7038) docs: fix csrf filter source origin note (envoyproxy#7041) Fix common typo: grcp -> grpc (envoyproxy#7040) snapshot (envoyproxy#7036) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
2 participants
Description: Add ProcessContext to server and expose to filters via FactoryContext
Risk Level: medium
Testing: new server test,
bazel test //test/...Docs Changes: n/a
Release Notes: (does this need one?)
Fixes #6969