-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Golang filter: provide a way to get all headers #29592
Comments
cc @doujiang24 @wangfakang @StarryVae golang's filter code-owners. |
I'm fine with I was thinking of an optimization: maybe we could use a higher performance type instead of
Compared to But, I haven't figured out a better way yet. So just a little worried about exposing the internal type to users. |
If the value is If |
yep, I agree that we should handle the repeated header correctly. |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions. |
I also think a method like GetAllHeaders would be useful. I don't know much about C++ programming so apologies if the following paragraph is uninformed. Presumably it would be faster to pass a data structure containing all the headers to the Golang plugin, rather than having the Golang plugin retrieve them from the Also, the issue with |
We cache the entire header map on the Golang side, in a single C++ Golang interchange.
okay, you means all headers exclude the special ones like scheme, path and etc, right? I think this might be a proper choice, it always do a copy for safety, thoughts?
|
OK, great - I wasn't aware of this.
Yes exactly, because the special ones are already available as attributes - envoy/contrib/golang/common/go/api/type.go Lines 138 to 141 in 5aadcb0
That seems perfect to me. Will the looping occur on the golang side or the C++ side? Just thinking the optimal thing would be to do any sort of loop on the C++ side so the whole operation is as fast as possible. |
Given the header map object is already cached, I wonder if the additional headers, i.e. the non-special ones, can't me made available in some sort of simple data structure in Golang - e.g. This prevents the user of the filter needing to create and populate such a data structure themselves for every request |
Golang side should be good enough, it's fast enough, no cgo call overhead.
It's a trade-off, since we already have a |
Fair enough, is it possible to access the existing cache without creating another |
it could be done in the current |
Oh I see, I'm beginning to understand how it all works now. So there's an existing But the OP was proposing to write a func which would return a copy of the entire existing I think that would be a good idea because it would be much faster. In some cases users will want to build other data structures using each header value as an attribute. With the existing |
Also, could the same method be added to the |
yep.
Do you really want all of the headers? maybe |
Edit, I made a mistake. With Envoy's native config you can set a route based on a header value, but you can't dynamically route to any cluster based on a header value - which is something you can do with the golang So I guess the general idea is you can set a header in the golang http plugin, and then route to a cluster using that header with the golang Personally, I think it would be great if you could retrieve all headers in the golang |
yep, Anyway, there is no disagreements about adding the new API:
Would you like to have a try? Seems it could be pure Golang implementation, no C++ code changes need. |
Ah yes! Thank you. So you can set a header, then route based on that header's value using Envoy's native config. So, if the golang
Therefore, could 1. be added to golang In the mean time I will try the golang implementation you mentioned in for the golang http plugin. |
yep. it should be a common API for HeaderMap.
Cool! |
I thought about it and I realised that it probably doesn't make sense to exclude the internal/special headers. This is because the user will most likely want all the headers - the only reason to exclude them is that in the case of The draft PR is here - do you have any thoughts? It's very simple. #33821 |
Noticed that |
okay, it's really not a good idea to design API by the performance that you thought. As said previously, headers already cached in Golang side, there shouldn't be much performance diff.
added some comments there. |
I'm happy to make an attempt on the Golang side of this, but I'll need the c++ parts to be done by someone else because I don't have experience with it |
maybe you can copy some code from golang http plugin, I'm happy to help if you need it. |
Yes I thought I might be able to do that.. OK, I'll give that a go in another PR and then tag you |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions. |
Title: Golang filter: provide a way to get all headers
Description:
We can add a new method
func (h *Header) GetAll(copy bool) map…
. The method takes a boolean to indicate whether we should return a copy of internal headers. If the headers are not copied, it is the user's duty to ensure the headers won't be written later or accessed concurrently.Here are the advantages:
Range
as we don't need to lock for longRange
if people want to dump all the headersIf this method is added, we can retire the
RangeWithCopy
because if people are afraid of the deadlock, they can useGetAll
with copy instead.The text was updated successfully, but these errors were encountered: