feat: validate APISIX resources in webhooks#393
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ADC validation: new Client.Validate and executor.Validate paths, APISIX-specific validate requests, structured ADC validation error types, controller helpers to prepare resources for validation, an adcAdmissionValidator wired into webhooks, and supporting unit and e2e tests that assert admission deny/fail-open behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant K8sClient as Kubernetes Client
participant Webhook as Admission Webhook
participant ADCValidator as adcAdmissionValidator
participant Controller as Controller Helper
participant ADCClient as ADC Client
participant Executor as HTTP Executor
participant ADCServer as ADC / APISIX Server
K8sClient->>Webhook: submit resource (Route/Consumer/TLS)
Webhook->>ADCValidator: Validate(resource)
ADCValidator->>Controller: Prepare*ForValidation(resource)
Controller-->>ADCValidator: TranslateContext
ADCValidator->>ADCClient: build Task / configs
ADCClient->>ADCClient: write temp sync file, record IO metrics
ADCClient->>Executor: Validate(config, args)
Executor->>ADCServer: POST /validate or POST /apisix/admin/configs/validate
ADCServer-->>Executor: 2xx/400 + payload
Executor->>ADCClient: structured validation result or infra error
ADCClient-->>ADCValidator: aggregated ADCValidationErrors or error
ADCValidator-->>Webhook: error (deny) or nil (allow)
Webhook-->>K8sClient: admission response (allowed/denied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T05:32:39Z"
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: 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.
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded. |
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T05:33:28Z"
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: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips.
- 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. |
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-07T05:54:46Z"
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:
- GRPCExactMethodMatching
- GRPCRouteHeaderMatching
- GRPCRouteListenerHostnameMatching
- GatewayModifyListeners
result: failure
statistics:
Failed: 4
Passed: 8
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 4 test failures.
- 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. |
There was a problem hiding this comment.
Pull request overview
This PR adds ADC-backed admission validation to APISIX-related validating webhooks so that explicit validation failures are rejected while infrastructure/backend issues remain fail-open, and extends unit/e2e test coverage for these flows.
Changes:
- Introduce a shared
adcAdmissionValidatorto translate resources and call ADC validation during admission for ApisixRoute, ApisixConsumer, Consumer, and ApisixTls. - Extend ADC client/executor with a validation pathway (including APISIX standalone
/apisix/admin/configs/validatesupport) and richer validation error types. - Update and add unit + e2e tests to assert admission rejections for invalid resources and duplicate credentials.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/webhook/helpers.go | Adds a helper for asserting admission denials and non-existence of rejected resources. |
| test/e2e/webhook/consumer.go | Adds an e2e case for rejecting duplicate Consumer credentials. |
| test/e2e/webhook/apisixtls.go | Switches missing-secret behavior to rejection and adds invalid TLS-material rejection coverage. |
| test/e2e/webhook/apisixroute.go | Adds e2e denial coverage for ADC validation failures and adjusts missing-ref expectations. |
| test/e2e/webhook/apisixconsumer.go | Switches missing-secret behavior to rejection and adds duplicate-credential denial coverage. |
| internal/webhook/v1/consumer_webhook_test.go | Adds unit test coverage for duplicate key-auth credential denial. |
| internal/webhook/v1/consumer_webhook.go | Wires ADC validation into admission and adds duplicate key-auth enforcement. |
| internal/webhook/v1/apisixtls_webhook_test.go | Adds ADC-deny unit test and improves test ingress class setup. |
| internal/webhook/v1/apisixtls_webhook.go | Wires ADC validation into ApisixTls admission validation. |
| internal/webhook/v1/apisixroute_webhook_test.go | Adds ADC-deny + fail-open unit tests for ApisixRoute. |
| internal/webhook/v1/apisixroute_webhook.go | Wires ADC validation into ApisixRoute admission validation. |
| internal/webhook/v1/apisixconsumer_webhook_test.go | Adds ADC-deny unit test for ApisixConsumer and improves ingress class setup. |
| internal/webhook/v1/apisixconsumer_webhook.go | Wires ADC validation into ApisixConsumer admission validation. |
| internal/webhook/v1/adc_validation_test.go | Adds shared ADC mock helpers for webhook validation tests. |
| internal/webhook/v1/adc_validation.go | Introduces shared admission validator translating resources and calling ADC validate with fail-open semantics. |
| internal/types/error.go | Adds typed ADC validation error structures for propagating explicit validation failures. |
| internal/controller/webhook_validation.go | Adds shared “prepare-for-validation” helpers used by admission validation translation. |
| internal/adc/client/executor_test.go | Adds unit tests for APISIX standalone validate payload conversion for SSL certificates. |
| internal/adc/client/executor.go | Adds ADC validation execution (incl. APISIX validate endpoint) and validation response parsing. |
| internal/adc/client/client.go | Adds Client.Validate to drive executor validation per resolved config. |
Comments suppressed due to low confidence (1)
internal/adc/client/executor.go:524
buildHTTPRequestlogs the entirereqBody(includingTokenand full translatedConfig) at V(1). With admission validation this will run on user writes, so tokens/credentials can end up in logs. Please redact/remove sensitive fields from logs (token, credentials, TLS material), or replace with a minimal summary (cacheKey, backend, resource counts).
reqBody := ADCServerRequest{
Task: ADCServerTask{
Opts: ADCServerOpts{
Backend: config.BackendType,
Server: strings.Split(serverAddr, ","),
Token: config.Token,
LabelSelector: labels,
IncludeResourceType: types,
TlsSkipVerify: ptr.To(!tlsVerify),
CacheKey: config.Name,
},
Config: *resources,
},
}
e.log.V(1).Info("prepared request body", "body", reqBody)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/webhook/v1/apisixconsumer_webhook_test.go (1)
45-66:⚠️ Potential issue | 🟡 MinorExtract
"apisix"to a constant to fix linter error.The string
"apisix"appears multiple times across test files. The linter flags this as agoconstviolation.🔧 Proposed fix
Add a constant at package level (e.g., in a shared test helper file or at the top of this file):
const defaultIngressClassName = "apisix"Then update the comparison and object creation:
for _, obj := range objects { ingressClass, ok := obj.(*networkingv1.IngressClass) - if ok && ingressClass.Name == "apisix" { + if ok && ingressClass.Name == defaultIngressClassName { hasManagedIngressClass = true break } } if !hasManagedIngressClass { managed = append(managed, &networkingv1.IngressClass{ ObjectMeta: metav1.ObjectMeta{ - Name: "apisix", + Name: defaultIngressClassName,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/webhook/v1/apisixconsumer_webhook_test.go` around lines 45 - 66, Introduce a package-level constant (e.g., const defaultIngressClassName = "apisix") and replace all literal "apisix" occurrences in this test with that constant: use defaultIngressClassName when comparing ingressClass.Name in the loop that sets hasManagedIngressClass and when constructing the &networkingv1.IngressClass{ObjectMeta: {Name: ..., Annotations: ...}, Spec: {Controller: config.ControllerConfig.ControllerName}} so the test no longer violates goconst; ensure the new constant is visible to other test files if they share the same package.
🧹 Nitpick comments (3)
internal/controller/webhook_validation.go (1)
44-49: Consider makingsupportsEndpointSliceconfigurable.The value
supportsEndpointSlice: trueis hardcoded here. If the cluster doesn't support EndpointSlices (older Kubernetes versions), this could cause issues during validation processing.Consider passing this as a parameter or deriving it from a shared configuration, similar to how the actual reconciler might determine this value at runtime. However, since EndpointSlices are GA since Kubernetes 1.21 and this is a recent codebase, hardcoding
trueis likely acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/webhook_validation.go` around lines 44 - 49, The hardcoded supportsEndpointSlice: true on the ApisixRouteReconciler instantiation in webhook_validation.go can break clusters without EndpointSlice support; change the construction of ApisixRouteReconciler to accept a configurable flag or derive it from shared cluster feature detection (e.g., read from a passed-in config struct or call a helper that checks API availability) and assign that value to ApisixRouteReconciler.supportsEndpointSlice instead of the literal true; update any callers that create this reconciler to pass the configured/derived boolean so validation honors cluster capabilities.internal/webhook/v1/consumer_webhook.go (1)
143-178: Consider performance impact of listing all Consumers on every validation.
validateDuplicateKeyAuthCredentialscallsv.Client.List(ctx, &consumers)without any field/label selector, fetching allConsumerresources cluster-wide. For clusters with many consumers, this could add significant latency to every create/update admission.Consider adding a label selector to filter by gateway reference, or implementing a cache/index if this becomes a bottleneck.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/webhook/v1/consumer_webhook.go` around lines 143 - 178, The code in validateDuplicateKeyAuthCredentials currently does a cluster-wide v.Client.List(ctx, &consumers) which is expensive; change the List invocation to only fetch relevant Consumers by constructing a selector (e.g., use client.MatchingLabels or client.MatchingFields) based on the consumer's gateway reference and namespace before calling v.Client.List, or switch to a cached/indexed lookup (via an informer/Lister) and query by the gatewayRef index; update the List call in validateDuplicateKeyAuthCredentials (and any helper that builds the query) so it only retrieves Consumers that could collide according to sameConsumerGatewayRef instead of listing all Consumers.internal/adc/client/executor.go (1)
328-340: Consider explicitly settingMinVersionon custom TLS client config.Go's default minimum TLS version is TLS 1.2, so the current code already meets the policy requirement. However, explicitly setting
MinVersion: tls.VersionTLS12improves code clarity and avoids reliance on runtime defaults that may change in future versions.Suggested fix
func (e *HTTPADCExecutor) newBackendHTTPClient(config adctypes.Config) *http.Client { transport := http.DefaultTransport.(*http.Transport).Clone() if !config.TlsVerify { if transport.TLSClientConfig == nil { - transport.TLSClientConfig = &tls.Config{} + transport.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + } + } else if transport.TLSClientConfig.MinVersion == 0 { + transport.TLSClientConfig.MinVersion = tls.VersionTLS12 } transport.TLSClientConfig.InsecureSkipVerify = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adc/client/executor.go` around lines 328 - 340, The TLS client config created in newBackendHTTPClient should explicitly set MinVersion to tls.VersionTLS12 instead of relying on runtime defaults; update the logic inside HTTPADCExecutor.newBackendHTTPClient where transport.TLSClientConfig is initialized so that if TLSClientConfig is nil you allocate it and set TLSClientConfig.MinVersion = tls.VersionTLS12 (and keep InsecureSkipVerify behavior unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adc/client/executor.go`:
- Around line 295-325: The log in buildAPISIXValidateRequest currently prints
sensitive data (config.Token and the request body containing creds/keys); change
it to log only non-sensitive metadata or a redacted payload: create a redacted
copy of the body (or call the type's MarshalLogObject/Redact method if
implemented) and replace config.Token with a placeholder (e.g., "<redacted>")
before logging, or remove body/token from the log entirely; update the logging
call in buildAPISIXValidateRequest (and the analogous logging in the other block
referred to) to use the redacted values so no raw tokens, consumer credentials,
certs, or private keys are emitted.
In `@internal/webhook/v1/adc_validation_test.go`:
- Around line 34-41: The helper withMockADCServer unnecessarily wraps the
already-typed http.HandlerFunc parameter when creating the test server; update
the call in withMockADCServer to pass handler directly to httptest.NewServer
(i.e., httptest.NewServer(handler)) and keep the rest of the function (t.Setenv,
t.Cleanup, return server.URL) unchanged to remove the redundant conversion.
In `@internal/webhook/v1/apisixconsumer_webhook.go`:
- Around line 47-63: The constructor stores initErr on
ApisixConsumerCustomValidator which causes a permanent hard-fail on any ADC init
error; remove the initErr field and stop storing that error in
NewApisixConsumerCustomValidator, and instead initialize adcValidator lazily
inside the validation entry points (e.g., the validator methods that run on
create/update) by calling newADCAdmissionValidator when adcValidator is nil; if
that init call returns an error, log the error with apisixConsumerLog and
proceed in "fail open" mode (do not block the request), ensuring reference to
the types/members ApisixConsumerCustomValidator,
NewApisixConsumerCustomValidator, adcValidator, and newADCAdmissionValidator so
you update the right places.
In `@internal/webhook/v1/apisixroute_webhook.go`:
- Around line 55-63: The validator currently stores initialization errors in
ApisixRouteCustomValidator.initErr (from newADCAdmissionValidator) and returns
them immediately in ValidateCreate/ValidateUpdate, causing permanent denials;
change ValidateCreate and ValidateUpdate on ApisixRouteCustomValidator to treat
initErr like runtime ADC unavailability: log the initErr via apisixRouteLog
(include context) and return nil (fail-open) instead of returning the initErr,
so runtime adcAdmissionValidator.Validate behavior and init-time failure are
consistent; keep the newADCAdmissionValidator and adcValidator fields as-is but
ensure all early-return paths reference initErr and use logging+nil return
rather than propagating the error.
In `@internal/webhook/v1/apisixtls_webhook.go`:
- Around line 82-87: The code currently returns v.initErr after collecting
warnings which turns ADC init failures into hard admission denials; change this
to fail-open: when v.initErr is non-nil, do not return the error — log or record
it but return the collected warnings with nil error and skip ADC validation
(i.e., do not call v.adcValidator.Validate when v.initErr != nil). Apply the
same change in the other similar block (lines 106-111) so ADC init/backend
problems do not permanently block admission or poison the validator.
In `@internal/webhook/v1/consumer_webhook.go`:
- Around line 49-65: The struct field initErr in ConsumerCustomValidator (and
the NewConsumerCustomValidator constructor) causes permanent admission denial
when initialization fails; remove the persistent initErr and instead handle
initialization failures by logging the error and returning a validator instance
that will operate in fail-open mode (e.g., set adcValidator to nil and proceed).
Update NewConsumerCustomValidator to not store initErr, and update any methods
that currently check initErr (such as Validate/Handle methods that may reside on
ConsumerCustomValidator) to treat a nil adcValidator as "uninitialized but
allow" rather than denying admission; keep reference.NewChecker and consumerLog
initialization as-is and ensure errors are logged via consumerLog.
- Around line 223-229: The code currently swallows json.Unmarshal errors when
parsing credential.Config.Raw into the local cfg struct and returns an empty
cfg.Key; change this so malformed JSON is surfaced: when
json.Unmarshal(credential.Config.Raw, &cfg) returns an error, return that error
(or at minimum log it with a contextual message including credential.Config.Raw)
instead of returning "", nil; update the function signature and callers as
required so callers can handle/report the unmarshalling error and include
references to json.Unmarshal, credential.Config.Raw, and cfg.Key when making the
change.
In `@test/e2e/webhook/apisixtls.go`:
- Around line 132-142: After recreating serverSecret with s.NewKubeTlsSecret,
wait until the cluster cache reflects the new Secret before creating the
ApisixTls to avoid race failures; implement a short polling loop that calls
s.GetResource("Secret", serverSecret) (or an existing helper like
s.GetKubeSecret) with a timeout/retry backoff and only proceed to call
s.CreateResourceFromString(tlsYAML) when the Secret is returned successfully,
ensuring you still check and Expect no error from that Get before continuing.
---
Outside diff comments:
In `@internal/webhook/v1/apisixconsumer_webhook_test.go`:
- Around line 45-66: Introduce a package-level constant (e.g., const
defaultIngressClassName = "apisix") and replace all literal "apisix" occurrences
in this test with that constant: use defaultIngressClassName when comparing
ingressClass.Name in the loop that sets hasManagedIngressClass and when
constructing the &networkingv1.IngressClass{ObjectMeta: {Name: ..., Annotations:
...}, Spec: {Controller: config.ControllerConfig.ControllerName}} so the test no
longer violates goconst; ensure the new constant is visible to other test files
if they share the same package.
---
Nitpick comments:
In `@internal/adc/client/executor.go`:
- Around line 328-340: The TLS client config created in newBackendHTTPClient
should explicitly set MinVersion to tls.VersionTLS12 instead of relying on
runtime defaults; update the logic inside HTTPADCExecutor.newBackendHTTPClient
where transport.TLSClientConfig is initialized so that if TLSClientConfig is nil
you allocate it and set TLSClientConfig.MinVersion = tls.VersionTLS12 (and keep
InsecureSkipVerify behavior unchanged).
In `@internal/controller/webhook_validation.go`:
- Around line 44-49: The hardcoded supportsEndpointSlice: true on the
ApisixRouteReconciler instantiation in webhook_validation.go can break clusters
without EndpointSlice support; change the construction of ApisixRouteReconciler
to accept a configurable flag or derive it from shared cluster feature detection
(e.g., read from a passed-in config struct or call a helper that checks API
availability) and assign that value to
ApisixRouteReconciler.supportsEndpointSlice instead of the literal true; update
any callers that create this reconciler to pass the configured/derived boolean
so validation honors cluster capabilities.
In `@internal/webhook/v1/consumer_webhook.go`:
- Around line 143-178: The code in validateDuplicateKeyAuthCredentials currently
does a cluster-wide v.Client.List(ctx, &consumers) which is expensive; change
the List invocation to only fetch relevant Consumers by constructing a selector
(e.g., use client.MatchingLabels or client.MatchingFields) based on the
consumer's gateway reference and namespace before calling v.Client.List, or
switch to a cached/indexed lookup (via an informer/Lister) and query by the
gatewayRef index; update the List call in validateDuplicateKeyAuthCredentials
(and any helper that builds the query) so it only retrieves Consumers that could
collide according to sameConsumerGatewayRef instead of listing all Consumers.
🪄 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: 1cf73cf2-2f73-47f0-8938-83053a0638bc
📒 Files selected for processing (20)
internal/adc/client/client.gointernal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/controller/webhook_validation.gointernal/types/error.gointernal/webhook/v1/adc_validation.gointernal/webhook/v1/adc_validation_test.gointernal/webhook/v1/apisixconsumer_webhook.gointernal/webhook/v1/apisixconsumer_webhook_test.gointernal/webhook/v1/apisixroute_webhook.gointernal/webhook/v1/apisixroute_webhook_test.gointernal/webhook/v1/apisixtls_webhook.gointernal/webhook/v1/apisixtls_webhook_test.gointernal/webhook/v1/consumer_webhook.gointernal/webhook/v1/consumer_webhook_test.gotest/e2e/webhook/apisixconsumer.gotest/e2e/webhook/apisixroute.gotest/e2e/webhook/apisixtls.gotest/e2e/webhook/consumer.gotest/e2e/webhook/helpers.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add ProviderTypeAPISIXStandalone skip guards to all ADC validation e2e tests so they are skipped on non-standalone backends that do not expose the /apisix/admin/configs/validate endpoint - Replace the 'duplicate credentials' consumer tests with invalid jwt-auth algorithm tests; APISIX validates plugin config schema in isolation and cannot detect cross-consumer key uniqueness, but it does reject unknown enum values (e.g. algorithm: INVALID_ALGO) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adc/client/executor.go`:
- Around line 328-341: The TLS config created in
HTTPADCExecutor.newBackendHTTPClient currently sets InsecureSkipVerify but does
not enforce a minimum TLS version; update newBackendHTTPClient to ensure the
TLSClientConfig has MinVersion = tls.VersionTLS12 (or higher) when creating or
augmenting transport.TLSClientConfig, and if transport.TLSClientConfig already
exists only set MinVersion when it is 0 to avoid overwriting explicit settings;
reference newBackendHTTPClient, HTTPADCExecutor, transport.TLSClientConfig and
tls.Config.MinVersion/tls.VersionTLS12.
🪄 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: 51318ffa-868c-4719-99ba-f7e37b438dbe
📒 Files selected for processing (4)
internal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/webhook/v1/adc_validation_test.gointernal/webhook/v1/apisixconsumer_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/webhook/v1/adc_validation_test.go
- internal/adc/client/executor_test.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e-test-k8s.yml (1)
70-70: Use an isolated kubeconfig file for self-hosted runner stability.Line 70 is correct, but exporting to the default kubeconfig path can cause state bleed across runs on
self-hosted. Prefer an explicitKUBECONFIGfile and persist it for downstream e2e steps.♻️ Suggested hardening
- name: Launch Kind Cluster env: KIND_NODE_IMAGE: kindest/node:v1.18.15 + KIND_NAME: apisix-ingress-cluster + KUBECONFIG: ${{ runner.temp }}/kind-${{ github.run_id }}.kubeconfig run: | make kind-up - kind export kubeconfig --name apisix-ingress-cluster + kind export kubeconfig --name "${KIND_NAME}" --kubeconfig "${KUBECONFIG}" + echo "KUBECONFIG=${KUBECONFIG}" >> "${GITHUB_ENV}" kubectl wait --for=condition=Ready nodes --all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test-k8s.yml at line 70, The current use of "kind export kubeconfig --name apisix-ingress-cluster" writes to the default kubeconfig and can leak state on self-hosted runners; fix by directing kind to an explicit file and exporting that path for downstream steps: create a dedicated kube dir (e.g. .kube/config in the workspace), set the KUBECONFIG environment variable to that file before invoking the "kind export kubeconfig --name apisix-ingress-cluster" command, and persist that env/file for subsequent e2e steps (via workflow env or step outputs) so all later steps use the isolated kubeconfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/e2e-test-k8s.yml:
- Line 70: The current use of "kind export kubeconfig --name
apisix-ingress-cluster" writes to the default kubeconfig and can leak state on
self-hosted runners; fix by directing kind to an explicit file and exporting
that path for downstream steps: create a dedicated kube dir (e.g. .kube/config
in the workspace), set the KUBECONFIG environment variable to that file before
invoking the "kind export kubeconfig --name apisix-ingress-cluster" command, and
persist that env/file for subsequent e2e steps (via workflow env or step
outputs) so all later steps use the isolated kubeconfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad9e436c-5a93-4381-acc1-55002624b4af
📒 Files selected for processing (1)
.github/workflows/e2e-test-k8s.yml
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Foo Bar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
The 'ApisixTls and Ingress with same certificate but different hosts' test was consistently failing due to two compounding issues: 1. Control-plane / data-plane propagation delay: the control plane confirms the api7.com SSL object, but APISIX data plane may not have loaded it yet when the HTTPS request is made. 2. Non-retryable Eventually block: NewAPISIXHttpsClient uses GinkgoT() as the httpexpect reporter. On TLS error, httpexpect calls GinkgoT().Fatalf() -> runtime.Goexit(), which exits the goroutine immediately. gomega's Eventually cannot retry because the goroutine is gone. Fix: replace the raw Eventually+httpexpect blocks with s.RequestAssert, which already exists in the scaffold and uses ErrorReporter (stores errors instead of calling FailNow). Transient TLS errors are now returned as retryable errors, letting Eventually poll until the data plane is fully ready. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When ApisixTls references secrets that do not exist yet, the webhook should warn (not deny). The ADC validator calls PrepareApisixTlsForValidation which in turn calls validateSecret, which returns NotFound and causes admission denial - breaking the original warn-on-missing-secret behavior. Fix: skip ADC validation when collectWarnings already detected missing secrets. The translator cannot load cert/key material in that case, so ADC validation would always fail anyway. The existing warnings are sufficient to inform the user. Also fix initErr fail-open: a validator initialization failure should allow admission (return warnings, nil) rather than hard-deny every write. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| warnings := v.collectWarnings(ctx, tls) | ||
| // Skip ADC validation when secrets are missing: the translator cannot | ||
| // load cert/key material, so validation would always fail. The missing- | ||
| // secret warnings are sufficient to inform the user. | ||
| if v.initErr != nil || len(warnings) > 0 { | ||
| return warnings, nil | ||
| } |
| @@ -45,16 +45,21 @@ func SetupApisixTlsWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, route) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -44,16 +44,21 @@ func SetupApisixRouteWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, consumer) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -45,16 +45,21 @@ func SetupApisixConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, consumer) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -44,16 +47,21 @@ func SetupConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
- fail-open on ADC validator init error in ApisixRoute, ApisixConsumer, Consumer webhooks - redact TLS payload from logs in buildAPISIXValidateRequest (log bodyLen only) - set TLS MinVersion to 1.2 in newBackendHTTPClient - return JSON unmarshal error in extractCredentialKey instead of swallowing it - default supportsEndpointSlice to false in PrepareApisixRouteForValidation - rename waitExponentialBackoffWithTimeout to waitConstantIntervalWithTimeout (Factor=1 is constant, not exponential) - restore WaitPodsAvailable timeout to 2 minutes to match original behavior - add cache-propagation sleep after recreating TLS Secret in e2e test - tighten expectAdmissionDenied to assert NotFound error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
buildAPISIXValidatePayload now includes GlobalRules (as a single GlobalRuleItem with generated ID) and PluginMetadata (per plugin name) in the validation request, ensuring APISIX validates the full config context rather than silently omitting these resources. Note: double ingress-class resolution in buildTask is retained to preserve the early-exit behavior (configs empty = no ADC server configured = skip validation). A proper single-resolution refactoring would require changes to PrepareApisixRouteForValidation signatures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…edentials Replace O(N) cluster-wide Consumer list with a field-indexed query on consumerGatewayRef, limiting the list to Consumers sharing the same gateway. This avoids redundant in-memory filtering via sameConsumerGatewayRef. Also remove the now-unused sameConsumerGatewayRef helper function. Update the test to register the consumerGatewayRef field index on the fake client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
internal/webhook/v1/consumer_webhook.go:47
- The kubebuilder webhook marker specifies failurePolicy twice with conflicting values (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for code generation and makes the intended failure policy unclear. Keep only one failurePolicy value in the marker.
| warnings := v.collectWarnings(ctx, tls) | ||
| // Skip ADC validation when secrets are missing: the translator cannot | ||
| // load cert/key material, so validation would always fail. The missing- | ||
| // secret warnings are sufficient to inform the user. | ||
| if v.initErr != nil || len(warnings) > 0 { | ||
| return warnings, nil | ||
| } |
| var errs types.ADCValidationErrors | ||
| for _, config := range task.Configs { | ||
| if config.BackendType == "" { | ||
| config.BackendType = c.defaultMode | ||
| } | ||
| if err := c.executor.Validate(ctx, config, args); err != nil { | ||
| var validationErr types.ADCValidationError | ||
| if errors.As(err, &validationErr) { | ||
| errs.Errors = append(errs.Errors, validationErr) |
|
|
||
| // Use the consumerGatewayRef field index to list only Consumers sharing the same gateway. | ||
| ns := consumer.Namespace | ||
| if consumer.Spec.GatewayRef.Namespace != nil && *consumer.Spec.GatewayRef.Namespace != "" { |
| @@ -45,16 +45,21 @@ func SetupApisixTlsWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| @@ -44,16 +44,21 @@ func SetupApisixRouteWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| @@ -45,16 +45,21 @@ func SetupApisixConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
Returning the unmarshal error would deny Consumers with malformed inline credential config, and also deny new Consumers when any existing Consumer has a malformed credential (because we'd fail while reading the existing consumer's keys). Log at debug level and skip the credential from duplicate detection instead, preserving the original fail-open behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| warnings := v.collectWarnings(ctx, tls) | ||
| // Skip ADC validation when secrets are missing: the translator cannot | ||
| // load cert/key material, so validation would always fail. The missing- | ||
| // secret warnings are sufficient to inform the user. | ||
| if v.initErr != nil || len(warnings) > 0 { | ||
| return warnings, nil |
| if v.initErr != nil || len(warnings) > 0 { | ||
| return warnings, nil | ||
| } |
| warnings := v.collectWarnings(ctx, route) | ||
| if v.initErr != nil { | ||
| apisixRouteLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation") | ||
| return warnings, nil | ||
| } | ||
| return warnings, v.adcValidator.Validate(ctx, route) |
| warnings := v.collectWarnings(ctx, route) | ||
| if v.initErr != nil { | ||
| apisixRouteLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation") | ||
| return warnings, nil | ||
| } | ||
| return warnings, v.adcValidator.Validate(ctx, route) |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | ||
|
|
||
| type ApisixTlsCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker |
| @@ -44,16 +44,21 @@ func SetupApisixRouteWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| @@ -45,16 +45,21 @@ func SetupApisixConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | ||
|
|
||
| type ConsumerCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker |
Restore original warn-on-missing-ref behavior for existing tests, and add update-path coverage for all four webhook resource types. Changes: - apisixconsumer_webhook.go: add missing 'if len(warnings) > 0' guard to skip ADC validation when references are missing (aligns with ApisixTls/ ApisixRoute pattern) - apisixconsumer.go: restore 'should warn on missing authentication secrets' (was incorrectly changed to deny) - apisixtls.go: restore 'should warn on missing TLS secrets' (was incorrectly changed to deny; webhook already admits when warnings exist) - apisixroute.go: add 'should reject route update that fails ADC validation' - apisixconsumer.go: add 'should reject consumer update that fails ADC validation' - apisixtls.go: add 'should reject TLS update with invalid certificate material' - consumer.go: add 'should reject consumer update that fails ADC validation' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove unused messageSubstrings variadic parameter from expectAdmissionDenied - Add nolint:dupl to warn-on-missing It blocks (structurally similar but different YAML) - Fix gofmt trailing newline in consumer_webhook.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add expectUpdateDenied helper: UPDATE denials leave the resource intact so the resource-not-found check in expectAdmissionDenied is wrong for update scenarios - Use expectUpdateDenied in all four UPDATE It blocks - Redesign ApisixTls UPDATE test: change the secret reference in the spec instead of swapping secret content; spec must actually change to trigger the UPDATE admission webhook Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both apisix and apisix-standalone backends now support /apisix/admin/configs/validate via ADC PR api7/adc#434. The CI uses apache/apisix:dev and ghcr.io/api7/adc:dev which include this support, so the skip guard is no longer needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the apisix-standalone backend bypassed the ADC server and called APISIX's /apisix/admin/configs/validate directly. With api7/adc#440, the ADC server now exposes a /validate endpoint (same input format as /sync) that handles both apisix-standalone and apisix backends uniformly. Changes: - Remove apisix-standalone special-case in runHTTPValidateForSingleServer; all backends now call ADC server POST /validate - Fix ADCValidateResult.ErrorMessage JSON tag: errorMessage -> message to match the ADC server response format from api7/adc#440 - Remove buildAPISIXValidateRequest, apisixValidateRequest, newBackendHTTPClient, buildAPISIXValidatePayload and helpers - Update unit tests accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ADC server registers PUT /validate (not POST), matching the same HTTP method used for PUT /sync. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APISIX validates plugin schemas but not credential schemas via /validate. Switch the invalid Consumer config in e2e tests to use spec.plugins with jwt-auth algorithm: INVALID_ALGO, which triggers proper ADC validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Testing
go test ./internal/adc/client ./internal/webhook/v1/...go test ./test/e2e/apisix -ginkgo.focus='Test (ApisixRoute|ApisixConsumer|Consumer|ApisixTls) Webhook'(currently 4 passed / 4 failed; route and consumer standalone validate payloads still need follow-up)Summary by CodeRabbit
New Features
Improvements
Tests