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

Add circuit breaker support for extension services. #6539

Conversation

clayton-gonsalves
Copy link
Contributor

@clayton-gonsalves clayton-gonsalves commented Jul 3, 2024

This PR adds support for configuring circuit breakers for extension service CRD's.
Fixes #6537

Note to reviewers

The method for configuring circuit breakers differs between extension services and regular services. For extension services, this configuration is managed directly on the extension service CRD rather than through service annotations.

This distinction arises from how Contour creates an Envoy cluster for a given extension service definition. An extension service CRD can be supported by multiple Kubernetes services, but for Envoy, these services are configured within a single Envoy cluster.

Combining annotations from multiple Kubernetes services is complex and can lead to a non-transparent user experience.

@clayton-gonsalves clayton-gonsalves requested a review from a team as a code owner July 3, 2024 20:10
@clayton-gonsalves clayton-gonsalves requested review from skriss and sunjayBhatia and removed request for a team July 3, 2024 20:10
@clayton-gonsalves clayton-gonsalves added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Jul 3, 2024
@sunjayBhatia sunjayBhatia requested review from a team, rajatvig and davinci26 and removed request for a team July 3, 2024 20:11
@clayton-gonsalves clayton-gonsalves force-pushed the add-circuit-breakers-for-extension-services branch from 2011476 to 85ed13f Compare July 3, 2024 20:12
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.66%. Comparing base (20d2ed9) to head (61e56a5).
Report is 13 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6539      +/-   ##
==========================================
+ Coverage   81.64%   81.66%   +0.02%     
==========================================
  Files         133      133              
  Lines       15873    15894      +21     
==========================================
+ Hits        12959    12980      +21     
  Misses       2620     2620              
  Partials      294      294              
Files Coverage Δ
cmd/contour/serve.go 22.66% <100.00%> (ø)
internal/dag/accessors.go 87.56% <100.00%> (+0.13%) ⬆️
internal/dag/dag.go 98.44% <ø> (ø)
internal/dag/extension_processor.go 95.95% <100.00%> (+0.66%) ⬆️
internal/dag/gatewayapi_processor.go 93.28% <ø> (ø)
internal/dag/httpproxy_processor.go 91.40% <ø> (ø)
internal/dag/ingress_processor.go 98.04% <ø> (ø)
internal/dag/policy.go 95.54% <100.00%> (+0.01%) ⬆️
internal/envoy/v3/cluster.go 96.41% <100.00%> (+0.04%) ⬆️
pkg/config/parameters.go 88.03% <ø> (ø)

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
@clayton-gonsalves clayton-gonsalves force-pushed the add-circuit-breakers-for-extension-services branch from 85ed13f to 9e946e2 Compare July 3, 2024 20:21
Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
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.

Thanks @clayton-gonsalves, this looks pretty good to me. I just had some nit-picky naming comments to try to keep things consistent/as short as possible. I think we should be able to get this into the upcoming release if you can address remaining feedback

apis/projectcontour/v1alpha1/extensionservice.go Outdated Show resolved Hide resolved
changelogs/unreleased/6539-clayton-gonsalves-minor.md Outdated Show resolved Hide resolved
apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved
apis/projectcontour/v1alpha1/extensionservice.go Outdated Show resolved Hide resolved
apis/projectcontour/v1alpha1/extensionservice.go Outdated Show resolved Hide resolved
internal/dag/dag.go Outdated Show resolved Hide resolved
internal/dag/dag.go Outdated Show resolved Hide resolved
internal/dag/dag.go Outdated Show resolved Hide resolved
internal/envoy/v3/cluster.go Outdated Show resolved Hide resolved
Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
@clayton-gonsalves clayton-gonsalves requested a review from skriss July 24, 2024 11:06
Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
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.

LGTM, thanks @clayton-gonsalves

@skriss skriss merged commit 601218d into projectcontour:main Jul 25, 2024
26 checks passed
geomacy pushed a commit to chaosbox/contour that referenced this pull request Aug 22, 2024
Closes projectcontour#6537.

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
SamMHD pushed a commit to SamMHD/contour that referenced this pull request Sep 8, 2024
Closes projectcontour#6537.

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>
Signed-off-by: Saman Mahdanian <saman@mahdanian.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Circuit breakers for extension services
3 participants