http: moving strict header checking into the http/1.1 codec#7601
http: moving strict header checking into the http/1.1 codec#7601mattklein123 merged 11 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@mattklein123 thoughts on the options above? |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. Yeah I agree (3) is the right approach for now. I think though we only ever need const access to the snapshot. That is how it works today. Though I'm not quite sure how you are going to actually post snapshot updates to non-worker threads. For now I think it's probably sufficient to just have a non-worker thread "current snapshot" which we acquire via a shared pointer read lock? And then the current snapshot is updated with a write lock? This would keep worker thread access lock free, and then in the future we could potentially allow non-worker threads to register for dispatcher updates if they run an event loop (or just implement a post like interface). WDYT?
/wait
| * | ||
| * @return true if this is a valid slot (called from a worker thread). | ||
| */ | ||
| virtual bool valid() PURE; |
There was a problem hiding this comment.
nit: currentThreadInitialized() or similar?
|
I think we can actually do one better, and make snapshot access actually const - looks like nothing but test code uses non-const and I can fix that up. If so we can combine 2 and 3 for a fairly simple solution. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This is a precursor to #7601 just to land the API change more quickly and make sure it sticks. Risk Level: Low Testing: existing unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Ok, I think this works better, but I want to see how CI is and probably do one more clean up pass before I remove the waiting tag. |
Optimally I think it's worth splitting it out if you don't mind, but it's not a big deal to me either way. |
|
I prefer smaller safer PRs so I'll close this off for a few and reopen once the runtime-only change has landed. |
Making runtime accessible for non-worker threads, and using the new accessor for runtime features. This allows the work done in #7601, moving the strict HTTP checks out of the HCM and into the codec, where the integration tests use them from client/server threads, and other downstream Envoys might use them from non-worker threads as well. Risk Level: High (affects runtime access for all runtime features) Testing: new unit tests, integration tests use in #7601 Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
changed the title
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Making runtime accessible for non-worker threads, and using the new accessor for runtime features. This allows the work done in #7601, moving the strict HTTP checks out of the HCM and into the codec, where the integration tests use them from client/server threads, and other downstream Envoys might use them from non-worker threads as well. Risk Level: High (affects runtime access for all runtime features) Testing: new unit tests, integration tests use in #7601 Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This is a precursor to envoyproxy#7601 just to land the API change more quickly and make sure it sticks. Risk Level: Low Testing: existing unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
…y#7695) (#27) Making runtime accessible for non-worker threads, and using the new accessor for runtime features. This allows the work done in envoyproxy#7601, moving the strict HTTP checks out of the HCM and into the codec, where the integration tests use them from client/server threads, and other downstream Envoys might use them from non-worker threads as well. Risk Level: High (affects runtime access for all runtime features) Testing: new unit tests, integration tests use in envoyproxy#7601 Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
…yproxy#26) This is a precursor to envoyproxy#7601 just to land the API change more quickly and make sure it sticks. Risk Level: Low Testing: existing unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: LSChyi <alan81920@gmail.com>
…y#7695) (envoyproxy#27) Making runtime accessible for non-worker threads, and using the new accessor for runtime features. This allows the work done in envoyproxy#7601, moving the strict HTTP checks out of the HCM and into the codec, where the integration tests use them from client/server threads, and other downstream Envoys might use them from non-worker threads as well. Risk Level: High (affects runtime access for all runtime features) Testing: new unit tests, integration tests use in envoyproxy#7601 Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: LSChyi <alan81920@gmail.com>
2 participants
As discovered on #7306 runtime checks don't work well outside of worker threads.
This leverages the fixed over in #7695 to move strict header checking to the codec.
Risk Level: Medium (messing with runtime, HTTP/1 parsing)
Testing: previously failing integration tests now pass
Docs Changes: n/a
Release Notes: n/a