Skip to content

alpha matching: ensure that extracted header values are not reused between streams#16721

Merged
snowp merged 12 commits intomainfrom
header-caching
Jun 7, 2021
Merged

alpha matching: ensure that extracted header values are not reused between streams#16721
snowp merged 12 commits intomainfrom
header-caching

Conversation

@snowp
Copy link
Copy Markdown
Contributor

@snowp snowp commented May 28, 2021

Fixes an issue where the extracted header value would be persisted between streams, resulting in incorrect
matching results. Streams that should not match would match due to the previous stream having a header
set that matches the result.

Risk Level: Medium, change to alpha feature
Testing: UT test case
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Snow Pettersen added 6 commits April 26, 2021 16:10
This was incorrectly causing the same values to be reused when the same match input was used between two streams
and the header was present in both streams.

Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
@snowp
Copy link
Copy Markdown
Contributor Author

snowp commented May 28, 2021

I think perhaps later on it would make sense to have the API return a implementable interface so that we can use that to wrap various string storage objects to avoid unnecessary copies, but this should fix the immediate issue.

Snow Pettersen added 2 commits May 30, 2021 18:07
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
data.onRequestHeaders(request_headers);

EXPECT_EQ(input.get(data).data_, "bar");
EXPECT_EQ(input.get(data).data_, "bar");
Copy link
Copy Markdown
Contributor

@dmitri-d dmitri-d May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below -- why a duplicate check is required? Perhaps add a comment here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code I think, let me remove one of them

@dmitri-d
Copy link
Copy Markdown
Contributor

Could you also add a test for Matcher::DataInputGetResult get when there's no requested header in the data? (to verify we get absl::nullopt here: https://github.com/envoyproxy/envoy/pull/16721/files#diff-2df46d0706764a215b8c3bf412d40761a2f7c02f8abb798aa929cad394e721b7R44)

@mattklein123
Copy link
Copy Markdown
Member

LMK when CI issues are resolved.

/wait

Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
@snowp
Copy link
Copy Markdown
Contributor Author

snowp commented Jun 3, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16721 (comment) was created by @snowp.

see: more, trace.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks looks good. Can you merge main?

/wait

Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
@snowp snowp merged commit 4d44deb into main Jun 7, 2021
@junr03 junr03 deleted the header-caching branch June 16, 2021 15:35
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
…tween streams (envoyproxy#16721)

This was incorrectly causing the same values to be reused when the same match input was used between two streams
and the header was present in both streams.

Signed-off-by: Snow Pettersen <snowp@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants