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

Add cluster extension #6118

Merged
merged 31 commits into from
Mar 8, 2019
Merged

Add cluster extension #6118

merged 31 commits into from
Mar 8, 2019

Conversation

HenryYYang
Copy link
Contributor

@HenryYYang HenryYYang commented Feb 27, 2019 •
edited
Loading

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add cluster factory to allow creating and registering custom cluster.
Risk Level: medium
Testing: Added unit tests and integration tests
Docs Changes:
Release Notes: Added.
#5780

@HenryYYang
Add cluster factory interface and implementation for the existing clu…
6dcb8de 
…sters

Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Add configurable factory
1ac5471 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Fix format issues
b4253c2 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
make cluster type oneof
3e056bb 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Fix format error.
f5420ee 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Add integration tests
d3a9004 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Updated comments and docs.
759e505 
Signed-off-by: Henry Yang <hyang@lyft.com>
@mattklein123 mattklein123 self-assigned this Feb 27, 2019
@HenryYYang
Merge remote-tracking branch 'origin' into add-cluster-extension
e7b1625 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
remove TestBase from new tests
ef1f54a 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Merge branch 'add-cluster-extension' of github.com:HenryYYang/envoy i…
cb3e0eb 
…nto add-cluster-extension

Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix formatting
35b2c86 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix formatting
b3c41e2 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix formatting
a5e19da 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix clang tidy
5311662 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix typo
6634824 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix clang-tidy
587b1d0 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Merge branch 'master' of github.com:envoyproxy/envoy into add-cluster…
ff123bf 
…-extension

Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
delete deprecated api
d16d0b9 
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
mattklein123 requested changes Mar 1, 2019
View reviewed changes
Copy link
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 very excited for this. @htuch can you do a quick API pass on this to see if this will meet Google needs? @envoyproxy/maintainers in case anyone else wants to take a look. @HenryYYang I will take a full pass once the function is deleted per offline discussion. Thank you!

/wait

.bazelrc Outdated Show resolved Hide resolved
DEPRECATED.md Outdated Show resolved Hide resolved
api/envoy/api/v2/cds.proto Outdated Show resolved Hide resolved
api/envoy/api/v2/cds.proto Outdated Show resolved Hide resolved
@htuch
Copy link
Member

htuch commented Mar 4, 2019

@mattklein123 this seems general enough to be useful for anything we'd want to do in terms of customer clusters. OTOH, I think our main interest today is making LB more extensible, which is orthogonal to cluster extensibility. I.e. we'd like to push out the existing LB to extensions and add our own, without needing to change from EdsCluster.

mattklein123
mattklein123 requested changes Mar 4, 2019
View reviewed changes
Copy link
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 this is great! Flushing some more comments.

/wait

api/envoy/api/v2/cds.proto Show resolved Hide resolved
docs/root/intro/arch_overview/service_discovery.rst Outdated Show resolved Hide resolved
include/envoy/upstream/cluster_factory.h Show resolved Hide resolved
include/envoy/upstream/cluster_factory.h Outdated Show resolved Hide resolved
source/common/upstream/cluster_factory_impl.cc Outdated Show resolved Hide resolved
source/common/upstream/cluster_factory_impl.cc Show resolved Hide resolved
source/common/upstream/cluster_factory_impl.h Outdated Show resolved Hide resolved
test/common/upstream/cluster_manager_impl_test.cc Show resolved Hide resolved
test/proto/cluster_factory_config.proto Outdated Show resolved Hide resolved
@HenryYYang
Fix PR feedbacks
0cf38a8 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Copy link
Contributor Author

🔨

@HenryYYang
Merge branch 'master' into add-cluster-extension
506f51b 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Remove TestBase
f75d362 
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
mattklein123 requested changes Mar 5, 2019
View reviewed changes
Copy link
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 this is really excellent work. A few small changes and then I think we are ready to ship!

/wait

docs/root/intro/arch_overview/service_discovery.rst Outdated Show resolved Hide resolved
docs/root/intro/version_history.rst Outdated Show resolved Hide resolved
include/envoy/upstream/cluster_factory.h Outdated Show resolved Hide resolved
source/common/upstream/cluster_factory_impl.h Outdated Show resolved Hide resolved
source/extensions/clusters/well_known_names.h Outdated Show resolved Hide resolved
test/common/upstream/cluster_factory_impl_test.cc Outdated Show resolved Hide resolved
test/integration/clusters/BUILD Outdated Show resolved Hide resolved
test/integration/clusters/custom_static_cluster.cc Outdated Show resolved Hide resolved
test/integration/custom_cluster_integration_test.cc Outdated Show resolved Hide resolved
@HenryYYang
Fix PR feedbacks
d51e41a 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Fix missing import
6ccc257 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Fix duplicate registration
0cb3806 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Split implementation and registration of custom static cluster out of…
1716d48 
… header file

Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
fix format
735524d 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
remove CUSTOM_CLUSTER_CONFIG
b06aca8 
Signed-off-by: Henry Yang <hyang@lyft.com>
HenryYYang and others added 2 commits March 7, 2019 16:16
@HenryYYang
Kick CI
0a53ccb 
Signed-off-by: Henry Yang <hyang@lyft.com>
@HenryYYang
Merge branch 'master' into add-cluster-extension
ccc50cb
@mattklein123 mattklein123 mentioned this pull request Mar 8, 2019
Closed
mattklein123
mattklein123 approved these changes Mar 8, 2019
View reviewed changes
Copy link
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.

Fantastic work! Thank you @HenryYYang!

@HenryYYang when you do your next PR for the new extension cluster, can you also take a look at https://github.com/envoyproxy/envoy/blob/master/REPO_LAYOUT.md and update it? I think we might be missing a few extensions, not just the clusters. Thank you!

@mattklein123 mattklein123 merged commit 95998b4 into envoyproxy:master Mar 8, 2019
@dfawley dfawley mentioned this pull request Mar 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HenryYYang @htuch @mattklein123 @alyssawilk