fix: remove redundant route.Hosts to prevent false diffs in ADC sync#392
Open
jarvis9443 wants to merge 2 commits intomasterfrom
Open
fix: remove redundant route.Hosts to prevent false diffs in ADC sync#392jarvis9443 wants to merge 2 commits intomasterfrom
jarvis9443 wants to merge 2 commits intomasterfrom
Conversation
The translator was setting hosts on both Route and Service for ApisixRoute resources. Since both APISIX and API7 EE backends support service-level hosts for route matching (APISIX routes inherit hosts from their parent service), the route-level hosts is redundant. For backends that don't support route-level hosts (e.g., API7 EE where hosts is a service-level concept), this causes a false diff every sync cycle: the local state has route.hosts but the remote state never will, triggering unnecessary PUT requests and audit log bloat. Remove route.Hosts assignment; service.Hosts remains as the canonical location for host matching. Ref: api7/rfcs#2 Ref: api7/adc#427
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRemoved per-route host assignment from the APISIXRoute translator, added unit tests verifying hosts are set at the service level (not per-route), and updated CI workflows to copy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/adc/translator/apisixroute_test.go (1)
53-54: Use a fatal length check before indexingservice.Routes[0].
assert.Lenis non-fatal, so a failed length assertion can still panic on the next line. Preferrequire.Lenhere.Suggested diff
import ( "testing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ - assert.Len(t, service.Routes, 1) + require.Len(t, service.Routes, 1) route := service.Routes[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adc/translator/apisixroute_test.go` around lines 53 - 54, The test uses assert.Len(t, service.Routes, 1) which is non-fatal and may let the subsequent indexing of service.Routes[0] panic; change this to a fatal check by using require.Len(t, service.Routes, 1) in the apisixroute_test.go test before accessing route := service.Routes[0], and add or ensure the testify/require import is present so the test fails immediately if the length is wrong.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/adc/translator/apisixroute_test.go`:
- Around line 53-54: The test uses assert.Len(t, service.Routes, 1) which is
non-fatal and may let the subsequent indexing of service.Routes[0] panic; change
this to a fatal check by using require.Len(t, service.Routes, 1) in the
apisixroute_test.go test before accessing route := service.Routes[0], and add or
ensure the testify/require import is present so the test fails immediately if
the length is wrong.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64bb35aa-caf4-4fe6-b379-157b6ca0c8a1
📒 Files selected for processing (2)
internal/adc/translator/apisixroute.gointernal/adc/translator/apisixroute_test.go
💤 Files with no reviewable changes (1)
- internal/adc/translator/apisixroute.go
Author
|
CI Status: lint ✅, unit tests ✅, misspell ✅, CRD docs ✅ The e2e test failures are a pre-existing infrastructure issue — the ADC dev Docker image ( |
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-10T05:50:51Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.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: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-10T05:51:00Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.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: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-10T06:12:22Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.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:
- GatewayModifyListeners
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
failedTests:
- HTTPRouteBackendProtocolWebSocket
result: failure
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 1
Passed: 10
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests failed with 1 test
failures.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures.
- core:
failedTests:
- GRPCExactMethodMatching
- GRPCRouteHeaderMatching
- GRPCRouteListenerHostnameMatching
- GatewayModifyListeners
result: failure
statistics:
Failed: 4
Passed: 8
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 4 test failures. |
The ghcr.io/api7/adc:dev Docker image now bundles main.cjs instead of main.js at the container root. Update all workflow files that extract the ADC binary via docker cp.
Author
|
Pushed CI fix: updated |
Author
|
CI Status update: 17/19 checks ✅ passed.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The translator sets
hostson both Route and Service when translating ApisixRoute resources. For backends that don't support route-level hosts (where hosts is a service-level concept), this causes a false diff every sync cycle: the local state hasroute.hostsbut the remote state never does, triggering unnecessary PUT requests.In production, this leads to massive audit log growth — one customer accumulated 8 GB / 4.2M records of redundant
UpdateServiceentries.Root Cause
In
buildRoute()(internal/adc/translator/apisixroute.go),route.Hostswas set torule.Match.Hosts. However,buildService()already setsservice.Hoststo the same value. Since routes inherit hosts from their parent service, the route-level hosts is redundant and can cause false diffs during config sync.Fix
Remove
route.Hosts = rule.Match.HostsfrombuildRoute().service.Hostsremains as the canonical location for host matching.CI Fix
Also fixes ADC binary extraction in CI workflows: the
ghcr.io/api7/adc:devDocker image now bundlesmain.cjsinstead ofmain.jsat the container root. Updatede2e-test.yml,e2e-test-k8s.yml, andapisix-e2e-test.yml.Testing
Added unit tests:
TestBuildRoute_HostsNotSet: verifies route.Hosts is not set after buildRouteTestBuildService_HostsSet: verifies service.Hosts is correctly setSummary by CodeRabbit
Bug Fixes
Tests