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.

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

internal/dag: add support for ExternalService clusters #2876

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Sep 3, 2020

Refactor the ExtensionService .Services field so that we have enough
information to build an EDS cluster load assignment. Dropping the use of
the HTTPProxy Service type means that the API aligns more closely with
the corresponding Envoy resources and we can build a single Envoy cluster
that balances across all the configured Kubernetes services. This wasn't
previously possible, since Envoy generally expects external services to
be a single cluster and has no provision for balancing across multiple
clusters in this part of its API.

Add a new dag.Processor implementation to build ExtensionService
objects in the DAG. These are root-level objects that generate Envoy
cluster resources. The .Services field emits a dag.ServiceCluster
object that generates an Envoy cluster load assignment resource to
balance traffic within the cluster.

This fixes #2713.

Signed-off-by: James Peach jpeach@vmware.com

@jpeach
Copy link
Contributor Author

jpeach commented Sep 3, 2020

Draft PR to pick up code coverage data. Still to do - add feature tests and fix debug logging.

@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from 6a1e207 to dda1a07 Compare September 3, 2020 06:42
@jpeach jpeach marked this pull request as ready for review September 3, 2020 06:43
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from dda1a07 to cc36128 Compare September 3, 2020 06:48
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from cc36128 to 73e4066 Compare September 3, 2020 06:51
@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #2876 into main will increase coverage by 0.25%.
The diff coverage is 94.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2876      +/-   ##
==========================================
+ Coverage   75.61%   75.86%   +0.25%     
==========================================
  Files          80       81       +1     
  Lines        5933     6025      +92     
==========================================
+ Hits         4486     4571      +85     
- Misses       1356     1361       +5     
- Partials       91       93       +2     
Impacted Files Coverage Δ
cmd/contour/serve.go 2.33% <0.00%> (-0.03%) ⬇️
internal/k8s/informers.go 0.00% <ø> (ø)
internal/dag/extension_processor.go 96.36% <96.36%> (ø)
internal/contour/cluster.go 100.00% <100.00%> (ø)
internal/dag/builder.go 91.91% <100.00%> (+0.25%) ⬆️
internal/dag/dag.go 96.21% <100.00%> (+0.05%) ⬆️
internal/envoy/cluster.go 100.00% <100.00%> (ø)
internal/featuretests/featuretests.go 93.40% <100.00%> (+0.10%) ⬆️
internal/dag/cache.go 95.71% <0.00%> (-0.72%) ⬇️

@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from 73e4066 to 53fb2ce Compare September 3, 2020 06:56
Copy link
Member

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

I'm not sure if you are ready for review or not @jpeach, your last comment was you were still working on tests, but I threw in some comments. =)

internal/dag/dag.go Outdated Show resolved Hide resolved
internal/contour/cluster.go Outdated Show resolved Hide resolved
internal/dag/dag.go Show resolved Hide resolved
internal/dag/extension_processor.go Show resolved Hide resolved
internal/envoy/cluster.go Outdated Show resolved Hide resolved
internal/envoy/cluster.go Outdated Show resolved Hide resolved
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from 53fb2ce to 0ea5d8e Compare September 6, 2020 21:38
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, with a couple of small changes. I'd like to see @stevesloka's things addressed, either with code changes or comments too, please.

internal/contour/cluster.go Outdated Show resolved Hide resolved
internal/dag/extension_processor.go Outdated Show resolved Hide resolved
internal/dag/extension_processor.go Outdated Show resolved Hide resolved
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch 2 times, most recently from 544416f to 03f6510 Compare September 7, 2020 05:33
@jpeach
Copy link
Contributor Author

jpeach commented Sep 7, 2020

@stevesloka @youngnick Addressed all review feedback.

@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch 2 times, most recently from 102bacb to d26c448 Compare September 7, 2020 22:08
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

looks good to me pending other comments

internal/dag/dag.go Outdated Show resolved Hide resolved
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from d26c448 to d949ef3 Compare September 9, 2020 00:32
@skriss
Copy link
Member

skriss commented Sep 9, 2020

sorry, gave you a couple small merge conflicts to resolve

@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from d949ef3 to f28ad9f Compare September 9, 2020 22:12
Refactor the ExtensionService `.Services` field so that we have enough
information to build an EDS cluster load assignment. Dropping the use of
the HTTPProxy `Service` type means that the API aligns more closely with
the corresponding Envoy resources and we can build a single Envoy cluster
that balances across all the configured Kubernetes services. This wasn't
previously possible, since Envoy generally expects external services to
be a single cluster and has no provision for balancing across multiple
clusters in this part of its API.

Add a new `dag.Processor` implementation to build ExtensionService
objects in the DAG. These are root-level objects that generate Envoy
cluster resources. The `.Services` field emits a `dag.ServiceCluster`
object that generates an Envoy cluster load assignment resource to
balance traffic within the cluster.

This fixes projectcontour#2713.

Signed-off-by: James Peach <jpeach@vmware.com>
@jpeach jpeach force-pushed the update-ExtensionServiceTarget branch from f28ad9f to 649e315 Compare September 10, 2020 04:23
@jpeach jpeach dismissed stevesloka’s stale review September 10, 2020 04:38

All feedback addressed.

@jpeach jpeach merged commit e529626 into projectcontour:main Sep 10, 2020
@jpeach jpeach deleted the update-ExtensionServiceTarget branch September 10, 2020 04:38
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.

Auth: Create Envoy clusters from ExtensionServices
4 participants