Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ecds: fix and test upstream HTTP filters #28904

Merged
merged 23 commits into from
Aug 15, 2023
Merged

ecds: fix and test upstream HTTP filters #28904

merged 23 commits into from
Aug 15, 2023

Conversation

ohadvano
Copy link
Contributor

@ohadvano ohadvano commented Aug 8, 2023 •
edited
Loading

Additional Description: Fixing a bug in ECDS for upstream HTTP filters and adding integration tests to verify cluster and router upstream HTTP filters with ECDS.
Risk Level: low
Testing: Integration tests
Docs Changes: ECDS docs, changelog
Release Notes: None
Platform Specific Features: None

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #28904 was opened by ohadvano.

see: more, trace.

@ohadvano
format
5a40d8d 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
add lds
4738501 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
rename stat prefix
979c375 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
fix and test
f94f068 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
fix test
71da59c 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
more tests
dc592c0 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
more tests
8c43561 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
docs
6451c8a 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
fix format
1f446a3 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
test
e7d969c 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
test
831cd82 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
empty
2a1c2b8 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
test
a9a85d3 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
test
13f76ae 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
fix flaky
c888f3e 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
refactor tests
f34b4ad 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano ohadvano changed the title ecds: integration tests for upstream HTTP filters ecds: fix and tests upstream HTTP filters Aug 11, 2023
@ohadvano ohadvano changed the title ecds: fix and tests upstream HTTP filters ecds: fix and test upstream HTTP filters Aug 11, 2023
@ohadvano
finish cluster tests
61d1592 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
parameterize tests for router and cluster filters
659b8b9 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
add tests when both router and cluster are configured
9ffdcf1 
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano ohadvano marked this pull request as ready for review August 11, 2023 15:08
@ohadvano
Copy link
Contributor Author

The PR is ready for review.
/assign @kyessenov
/assign @htuch
I'll appreciate if you could review this

@ohadvano
defer finalinze
676e80b 
Signed-off-by: ohadvano <ohadvano@gmail.com>
kyessenov
kyessenov requested changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you for working on this, especially the tests. I have only one comment - to move the filter config provider to the cluster manager, since that's the logical place for it.

docs/root/configuration/overview/extension.rst Outdated Show resolved Hide resolved
source/common/filter/config_discovery_impl.h Show resolved Hide resolved
source/common/upstream/upstream_impl.cc Show resolved Hide resolved
test/integration/filters/add_header_filter.cc Outdated Show resolved Hide resolved
test/integration/filters/add_header_filter.cc Outdated Show resolved Hide resolved
test/integration/filters/add_header_filter.proto Show resolved Hide resolved
test/integration/upstream_http_filter_integration_test.cc Outdated Show resolved Hide resolved
@ohadvano
add todo, use config proto
e28cfcc 
Signed-off-by: ohadvano <ohadvano@gmail.com>
kyessenov
kyessenov approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

LGTM

@kyessenov
Copy link
Contributor

@htuch Do you mind to provide a secondary approval as a senior maintainer? This PR touches some core code.

htuch
htuch approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this important fix!

@htuch htuch merged commit f9ffd76 into envoyproxy:main Aug 15, 2023
115 checks passed
@ohadvano ohadvano deleted the ecds_upstream_http branch August 15, 2023 05:41
phlax added a commit to phlax/envoy that referenced this pull request Aug 17, 2023
@phlax
Revert "ecds: fix and test upstream HTTP filters (envoyproxy#28904)"
d7da665 
This reverts commit f9ffd76.

Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit to phlax/envoy that referenced this pull request Aug 17, 2023
@phlax
Revert "ecds: fix and test upstream HTTP filters (envoyproxy#28904)"
ee149b5 
This reverts commit f9ffd76.

Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyessenov kyessenov kyessenov approved these changes

@htuch htuch htuch approved these changes

Assignees

@kyessenov kyessenov

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ohadvano @kyessenov @htuch