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

Fix for specifying a health check port with an ExternalName Service #6230

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

yangyy93
Copy link
Member

close #6229

Signed-off-by: yangyang <yang.yang@daocloud.io>
@yangyy93 yangyy93 requested a review from a team as a code owner February 29, 2024 08:31
@yangyy93 yangyy93 requested review from skriss and sunjayBhatia and removed request for a team February 29, 2024 08:31
@sunjayBhatia sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team February 29, 2024 08:32
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.33%. Comparing base (a16e749) to head (51da8fb).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6230   +/-   ##
=======================================
  Coverage   81.33%   81.33%           
=======================================
  Files         133      133           
  Lines       15772    15775    +3     
=======================================
+ Hits        12828    12831    +3     
  Misses       2650     2650           
  Partials      294      294           
Files Coverage Δ
internal/envoy/v3/endpoint.go 100.00% <100.00%> (ø)

xds.ClusterLoadAssignmentName(
types.NamespacedName{Name: service.Weighted.ServiceName, Namespace: service.Weighted.ServiceNamespace},
service.Weighted.ServicePort.Name,
),
SocketAddress(service.ExternalName, int(service.Weighted.ServicePort.Port)),
)
if service.Weighted.ServicePort.Port != service.Weighted.HealthPort.Port {
cla.Endpoints[0].LbEndpoints[0].GetEndpoint().HealthCheckConfig = HealthCheckConfig(service.Weighted.HealthPort.Port)
Copy link
Member

Choose a reason for hiding this comment

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

This looks OK to me but I had to stop for a bit when reading - we know we always assign exactly 1 address and therefore it is safe to access Endpoints[0].LbEndpoints[0], right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah we pass only one address to ClusterLoadAssignment above so the returned cla should have one endpoint entry, but lets add another test of just the ExternalNameClusterLoadAssignment function to endpoint_test.go to be sure

@sunjayBhatia sunjayBhatia changed the title fix #6229 Fix for specifying a health check port with an ExternalName Service Feb 29, 2024
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Feb 29, 2024
Copy link
Member

@sunjayBhatia sunjayBhatia 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 just needs another test and changelog file

Signed-off-by: yangyang <yang.yang@daocloud.io>
Signed-off-by: yangyang <yang.yang@daocloud.io>
@sunjayBhatia sunjayBhatia merged commit 53e9159 into projectcontour:main Mar 4, 2024
26 checks passed
lubronzhan pushed a commit to lubronzhan/contour that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExternalName service health check port does not take effect
3 participants