Skip to content

fix: should not return when service type is ExternalName #203

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

Merged
merged 2 commits into from
Jul 16, 2025

Conversation

ronething
Copy link
Contributor

@ronething ronething commented Jul 15, 2025

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

ref: apache/apisix-ingress-controller#2468

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@ronething ronething changed the title fix: should not return when service type is ExternalName (#2468) fix: should not return when service type is ExternalName Jul 15, 2025
@ronething ronething requested a review from Copilot July 15, 2025 03:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a bug where ExternalName services caused the reconciler to exit early, preventing additional backendRefs from being processed, and adds an end-to-end test to verify load balancing across ExternalName services.

  • Changed the reconciler to continue instead of return when encountering ServiceTypeExternalName
  • Added an E2E test that spins up ExternalName services, a BackendTrafficPolicy, and an HTTPRoute, then validates round-robin behavior
  • Defined a servicesSpec block in tests to create two ExternalName services and associated HTTPRoute

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/e2e/gatewayapi/httproute.go New E2E test for HTTPRoute load balancing with ExternalName types
internal/controller/httproute_controller.go Replace return with continue when handling ExternalName services
Comments suppressed due to low confidence (1)

internal/controller/httproute_controller.go:504

  • Add a unit test for processHTTPRouteBackendRefs covering the ServiceTypeExternalName path to ensure these services get added to tctx.Services without exiting the loop.
			continue

Comment on lines +1792 to +1793
time.Sleep(5 * time.Second)

Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Instead of a fixed time.Sleep, use a polling loop or Gomega's Eventually to wait for the HTTPRoute to become ready. This reduces flakiness and can shorten test run time.

Suggested change
time.Sleep(5 * time.Second)
Eventually(func() bool {
resp, err := s.NewAPISIXClient().GET("/headers").Expect().Raw()
if err != nil {
return false
}
return resp.StatusCode == http.StatusOK
}, 30*time.Second, 1*time.Second).Should(BeTrue(), "HTTPRoute should become ready")

Copilot uses AI. Check for mistakes.

Comment on lines +1799 to +1818
for i := 0; i < totalRequests; i++ {
resp := s.NewAPISIXClient().GET("/headers").Expect().Status(http.StatusOK)

// Parse JSON response to get the Host header
var responseBody map[string]any
resp.JSON().Decode(&responseBody)

if headers, ok := responseBody["headers"].(map[string]any); ok {
var host string
if host, ok = headers["Host"].(string); !ok {
host, ok = headers["host"].(string)
}
if ok && host != "" {
upstreamHosts[host]++
}
Expect(ok).To(BeTrue(), "Host header should be present")
Expect(host).Should(Or(Equal("httpbin.org"), Equal("mock.api7.ai")))
}
time.Sleep(100 * time.Millisecond) // Small delay between requests
}
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Avoid fixed delays inside the request loop. If pacing is needed, consider using parallel requests with a timeout or parameterizing the delay to speed up the suite.

Suggested change
for i := 0; i < totalRequests; i++ {
resp := s.NewAPISIXClient().GET("/headers").Expect().Status(http.StatusOK)
// Parse JSON response to get the Host header
var responseBody map[string]any
resp.JSON().Decode(&responseBody)
if headers, ok := responseBody["headers"].(map[string]any); ok {
var host string
if host, ok = headers["Host"].(string); !ok {
host, ok = headers["host"].(string)
}
if ok && host != "" {
upstreamHosts[host]++
}
Expect(ok).To(BeTrue(), "Host header should be present")
Expect(host).Should(Or(Equal("httpbin.org"), Equal("mock.api7.ai")))
}
time.Sleep(100 * time.Millisecond) // Small delay between requests
}
var wg sync.WaitGroup
var mu sync.Mutex
for i := 0; i < totalRequests; i++ {
wg.Add(1)
go func() {
defer wg.Done()
resp := s.NewAPISIXClient().GET("/headers").Expect().Status(http.StatusOK)
// Parse JSON response to get the Host header
var responseBody map[string]any
resp.JSON().Decode(&responseBody)
if headers, ok := responseBody["headers"].(map[string]any); ok {
var host string
if host, ok = headers["Host"].(string); !ok {
host, ok = headers["host"].(string)
}
if ok && host != "" {
mu.Lock()
upstreamHosts[host]++
mu.Unlock()
}
Expect(ok).To(BeTrue(), "Host header should be present")
Expect(host).Should(Or(Equal("httpbin.org"), Equal("mock.api7.ai")))
}
}()
}
wg.Wait()

Copilot uses AI. Check for mistakes.

Copy link

github-actions bot commented Jul 15, 2025

conformance test report - apisix-standalone mode

apiVersion: gateway.networking.k8s.io/v1
date: "2025-07-16T12:04:16Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    failedTests:
    - HTTPRouteCrossNamespace
    result: failure
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 1
      Passed: 31
      Skipped: 1
  name: GATEWAY-HTTP
  summary: Core tests failed with 1 test failures.

Copy link

github-actions bot commented Jul 15, 2025

conformance test report - apisix mode

apiVersion: gateway.networking.k8s.io/v1
date: "2025-07-16T12:04:34Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    failedTests:
    - HTTPRouteCrossNamespace
    result: failure
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 1
      Passed: 31
      Skipped: 1
  name: GATEWAY-HTTP
  summary: Core tests failed with 1 test failures.

Copy link

github-actions bot commented Jul 15, 2025

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2025-07-16T10:19:06Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: partial
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 0
      Passed: 32
      Skipped: 1
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 1 test skips.

@ronething ronething merged commit a5c4fb7 into release-v2-dev Jul 16, 2025
18 of 20 checks passed
@ronething ronething deleted the fix/externalname branch July 16, 2025 12:21
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.

2 participants