Pass plugin key from embedders to PluginBase. #178
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Takeshi Yoneda <[email protected]>
PiotrSikora
reviewed
Jul 11, 2021
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
mathetake
changed the title
lizan
pushed a commit
to envoyproxy/envoy
that referenced
this pull request
Jul 12, 2021
This is the follow up on #16795. ## Bug description If the same plugin/rootcontext is shared across multiple listeners, then there is a race in listener information. Before #16795, this was not a problem for stream contexts since given listener information is from *where* it is created (`Plugin: plugin_` member created in the factory instance) https://github.com/envoyproxy/envoy/pull/16795/files#diff-90e133f83d91e5c22d1f7c7e8c0cf8017536a23765978bf81c7efd0482be0d81L43 But after that being merged, the `Plugin` passed to stream context constructor is the one from `PluginHandle::plugin()` which is potentially shared across multiple listeners. For root contexts, this has been a problem since the beginning because the root context has been already potentially shared across multiple listeners, and pointing to the listener's info which is created first. ## Change in this PR Combined with proxy-wasm/proxy-wasm-cpp-host#178, we explicitly include listener information into the plugin key so that if two plugins have different info to each other, different plugin instances will be created. ## Note - Maybe we could drop the support for sharing 1 Plugin across multiple listeners (or filters), then this race condition would be resolved in a different way. I think it is worth discussion. - This race is originally reported by Istio istio/proxy#3395 (comment) Signed-off-by: Takeshi Yoneda <[email protected]>
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
) This is the follow up on envoyproxy#16795. ## Bug description If the same plugin/rootcontext is shared across multiple listeners, then there is a race in listener information. Before envoyproxy#16795, this was not a problem for stream contexts since given listener information is from *where* it is created (`Plugin: plugin_` member created in the factory instance) https://github.com/envoyproxy/envoy/pull/16795/files#diff-90e133f83d91e5c22d1f7c7e8c0cf8017536a23765978bf81c7efd0482be0d81L43 But after that being merged, the `Plugin` passed to stream context constructor is the one from `PluginHandle::plugin()` which is potentially shared across multiple listeners. For root contexts, this has been a problem since the beginning because the root context has been already potentially shared across multiple listeners, and pointing to the listener's info which is created first. ## Change in this PR Combined with proxy-wasm/proxy-wasm-cpp-host#178, we explicitly include listener information into the plugin key so that if two plugins have different info to each other, different plugin instances will be created. ## Note - Maybe we could drop the support for sharing 1 Plugin across multiple listeners (or filters), then this race condition would be resolved in a different way. I think it is worth discussion. - This race is originally reported by Istio istio/proxy#3395 (comment) Signed-off-by: Takeshi Yoneda <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR allows embedders to pass plugin_key for PluginBases in order for it to include host specific information (e.g. listener info in Envoy).
ref envoyproxy/envoy#17243 (comment)
Signed-off-by: Takeshi Yoneda [email protected]