fix: rebuild the ADC baseline on leader acquisition#430
Open
AlinsRan wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe ADC version requirement is updated, sync requests gain conditional cache bypassing, and standalone clients rebuild baselines after leadership changes or ChangesADC cache recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant ADCClient
participant Executor
participant APISIX
Provider->>ADCClient: InvalidateADCCache()
ADCClient->>Executor: Execute sync with BypassCache
Executor->>APISIX: Submit sync request
APISIX-->>ADCClient: Return sync result or conf_version rejection
ADCClient->>Executor: Retry with BypassCache=true
Executor->>APISIX: Rebuild baseline and submit sync
APISIX-->>ADCClient: Return retry result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adc/client/client.go`:
- Around line 397-419: Extract the stale conf_version recovery sequence
currently in the per-config path into a shared helper, preserving its logging,
metric recording, cache bypass, retry, and error-reporting behavior. Invoke this
helper for the global-rule ADC execution before that branch returns, as well as
from the existing config path, so both execution paths recover from conflicts.
Add a regression test covering global_rule synchronization with a stale
conf_version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5113521-b802-433f-b553-212fb9476afb
📒 Files selected for processing (8)
Makefileapi/adc/types.godocs/en/latest/developer-guide.mdinternal/adc/client/client.gointernal/adc/client/executor.gointernal/adc/client/executor_test.gotest/e2e/apisix/conf_version.gotest/e2e/scaffold/apisix_deployer.go
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-14T05:54:17Z"
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:
result: partial
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 0
Passed: 32
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 partially succeeded with 1 test skips. 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-07-14T05:54:21Z"
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-07-14T06:11:55Z"
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
statistics:
Failed: 1
Passed: 11
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 1 test failures.
- core:
failedTests:
- GatewayModifyListeners
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:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures. |
AlinsRan
force-pushed
the
fix/adc-conf-version-rebuild
branch
from
July 13, 2026 09:21
a9343b4 to
e0a7361
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/adc/client/client.go (1)
384-407: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftGlobal-rule sync path still has no conflict recovery.
This branch executes ADC (
c.executor.Execute(ctx, config, args)) and handles its error, but unlike the non-global path added at lines 440-483, it never setsconfig.BypassCachebased on baseline currency and never retries viarejectedByDataPlane. Any config whose task includesglobal_rulewill still permanently fail on a staleconf_versioninstead of self-healing. This was already flagged on a prior commit of this file; it remains unaddressed in the current diff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adc/client/client.go` around lines 384 - 407, Update the global-rule execution flow around c.executor.Execute and align it with the non-global path’s conflict recovery: set config.BypassCache based on baseline currency, then retry rejected stale-conf_version executions through rejectedByDataPlane. Preserve the existing status, error collection, and execution-duration metrics while allowing global_rule tasks to self-heal.internal/adc/client/executor.go (1)
424-448: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrefer a 400 data-plane status when aggregating standalone endpoint failures.
dataPlaneStatuscurrently takes the first failing endpoint’s status, so a later400 Bad Requestcan be hidden behind an earlier non-400 failure and skip the bypassCache retry inrejectedByDataPlane.🐛 Proposed fix
for _, ep := range result.EndpointStatus { if !ep.Success { failedEndpoints = append(failedEndpoints, fmt.Sprintf("%s: %s", ep.Server, ep.Reason)) - if dataPlaneStatus == 0 { + if dataPlaneStatus == 0 || ep.Response.Status == http.StatusBadRequest { dataPlaneStatus = ep.Response.Status } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adc/client/executor.go` around lines 424 - 448, Update the endpoint-status aggregation in the standalone-mode handling to prefer HTTP 400 when selecting dataPlaneStatus. In the loop over result.EndpointStatus, continue recording a failure status, but allow a later failing endpoint with status 400 to replace an earlier non-400 status so rejectedByDataPlane can trigger its bypassCache retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/adc/client/client.go`:
- Around line 384-407: Update the global-rule execution flow around
c.executor.Execute and align it with the non-global path’s conflict recovery:
set config.BypassCache based on baseline currency, then retry rejected
stale-conf_version executions through rejectedByDataPlane. Preserve the existing
status, error collection, and execution-duration metrics while allowing
global_rule tasks to self-heal.
In `@internal/adc/client/executor.go`:
- Around line 424-448: Update the endpoint-status aggregation in the
standalone-mode handling to prefer HTTP 400 when selecting dataPlaneStatus. In
the loop over result.EndpointStatus, continue recording a failure status, but
allow a later failing endpoint with status 400 to replace an earlier non-400
status so rejectedByDataPlane can trigger its bypassCache retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2716049f-d5cb-4be4-8d9b-9c8d6fc3b394
📒 Files selected for processing (10)
Makefileapi/adc/types.godocs/en/latest/developer-guide.mdinternal/adc/client/client.gointernal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/provider/apisix/provider.gointernal/types/error.gotest/e2e/apisix/conf_version.gotest/e2e/scaffold/apisix_deployer.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/en/latest/developer-guide.md
- Makefile
- test/e2e/apisix/conf_version.go
- test/e2e/scaffold/apisix_deployer.go
AlinsRan
force-pushed
the
fix/adc-conf-version-rebuild
branch
from
July 14, 2026 01:02
e0a7361 to
52d98df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adc/client/client.go`:
- Around line 435-475: The baseline check, execution, and mark-current sequence
in applySync must be serialized per cache key. Replace the shared-lock
protection used by applySync with an exclusive lock or equivalent per-key guard
covering baselineIsCurrent, Execute (including retry), and markBaselineCurrent,
while preserving concurrency for different keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 214a3c9d-6fec-44ae-b2fb-d75898cf4969
📒 Files selected for processing (9)
Makefileapi/adc/types.godocs/en/latest/developer-guide.mdinternal/adc/client/client.gointernal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/provider/apisix/provider.gotest/e2e/apisix/conf_version.gotest/e2e/scaffold/apisix_deployer.go
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- internal/provider/apisix/provider.go
- test/e2e/apisix/conf_version.go
- test/e2e/scaffold/apisix_deployer.go
- internal/adc/client/executor.go
- docs/en/latest/developer-guide.md
AlinsRan
changed the title
The ADC server runs as a sidecar that outlives the controller process. Losing the
lease terminates the manager container but not the sidecar, so the baseline ADC
holds per cacheKey -- the last synced content plus the conf_version it generated --
survives a leadership change. Once the pod is elected again, ADC diffs against that
frozen snapshot and carries over conf_versions older than the ones the leader in
between has already pushed. APISIX standalone requires *_conf_version to be
monotonic and rejects the request:
upstreams_conf_version must be greater than or equal to (1779434128737)
The data plane validates the configuration as a whole, so every later sync keeps
failing and no update lands at all, endpoint updates included. Backends running a
single pod then return 502/504 once that pod is rescheduled, while multi-pod
backends keep serving on their remaining endpoints.
Winning the election is the one moment a baseline from an earlier term can enter the
picture, so that is where to answer it: the provider, which only runs once elected,
puts every baseline in doubt, and the first sync of each cacheKey re-derives it from
the data plane through ADC's bypassCache before anything is pushed from it. Only a
sync ADC accepts settles the question, so a rebuild that never landed is attempted
again rather than assumed.
Prevention cannot cover a desync no leadership change explains -- another writer on
this data plane, something we did not foresee -- and the only way any of that shows
itself is the data plane refusing a conf_version. So that rejection rebuilds the
baseline and pushes once more. It is recognised by the field it names rather than by
the sentence around it: conf_version belongs to the standalone admin API, we send
those keys ourselves, while the prose around them is APISIX's to reword. This is a
net, not the fix: nothing else rebuilds on it, and the reported bug does not come
back if it ever stops firing.
bypassCache needs ADC >= 0.27.0, so bump ADC_VERSION to 0.27.1. The sidecar image
the chart deploys has to move with it, or the rebuild is answered with a schema
error instead of healing.
The e2e bumps every *_conf_version the data plane holds straight through its Admin
API -- what the leader in between would have left behind -- and then expects an
Ingress update to still reach the data plane. Nothing restarts during the spec, so
what it exercises is the safety net. It reproduces the bug without the fix.
AlinsRan
force-pushed
the
fix/adc-conf-version-rebuild
branch
from
July 14, 2026 05:40
52d98df to
817216d
Compare
nic-6443
approved these changes
Jul 15, 2026
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
2 participants
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 ADC server is a sidecar that outlives the controller process. When the pod loses the lease, the manager container restarts but the sidecar does not, so ADC keeps its per-
cacheKeybaseline — the last synced content plus theconf_versionit generated.Meanwhile another pod, as leader, bumps
*_conf_versionin APISIX. When the first pod is elected again, ADC diffs against its stale baseline and pushes aconf_versionolder than what APISIX now holds. APISIX standalone requires it to be monotonic and rejects the whole push:From then on every sync fails and nothing lands — single-pod backends 502/504 once rescheduled. Restarting the pod clears it (the sidecar dies with it); the manager restarting on its own does not.
Fix
ADC ≥ 0.27.0 accepts
bypassCacheon/sync: it drops the baseline and re-derives it from the data plane. The baseline is rebuilt in two places:provider.Start()only runs once elected, and the first sync of each cacheKey rebuilds before pushing. Staleness can only enter on a leadership change, so this is where it is answered.conf_versionrejection — a safety net for a desync no leadership change explains (another writer, an evicted cache entry). Matched by the field name, not the message wording.Everything else is untouched: unrelated errors don't trigger a rebuild, non-standalone mode has no
conf_version, and a normal sync is byte-for-byte what it was (bypassCacheisomitempty).ADC_VERSION→ 0.27.1.Important
The ADC sidecar image (from the chart, not this repo) must also move to ≥ 0.27.0, or the rebuild is rejected as an unknown field and the fix is a no-op.
Test
test/e2e/apisix/conf_version.goreproduces the bug: bump every*_conf_versionvia the Admin API, then assert an Ingress update still lands. Fails without the fix, passes with it. Verified against APISIX 3.17.0 + ADC 0.27.1.