Skip to content

feat: validate APISIX resources in webhooks#393

Open
AlinsRan wants to merge 37 commits intomasterfrom
feat/webhook-adc-validation
Open

feat: validate APISIX resources in webhooks#393
AlinsRan wants to merge 37 commits intomasterfrom
feat/webhook-adc-validation

Conversation

@AlinsRan
Copy link
Copy Markdown
Contributor

@AlinsRan AlinsRan commented Apr 27, 2026

Summary

  • add shared ADC-backed admission validation for APISIX CRD webhooks
  • deny writes on explicit validation failures while keeping transport and backend errors fail-open
  • add unit and e2e coverage for ApisixRoute, ApisixConsumer, Consumer, and ApisixTls webhook validation flows

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

    • ADC-based admission validation added for Routes, Consumers, and TLS; supports standalone APISIX admin validation.
    • Executors now perform per-backend validation and aggregate server-specific validation results.
  • Improvements

    • Admission rejects invalid secrets/configs more strictly; preserves fail-open for infra errors.
    • Duplicate key-auth credential detection across same gateway prevents conflicting Consumers.
  • Tests

    • Expanded unit and e2e coverage for ADC flows, payload translation, and admission-deny scenarios.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
ADC client & executor
internal/adc/client/client.go, internal/adc/client/executor.go, internal/adc/client/executor_test.go
Add Client.Validate; extend ADCExecutor with Validate and implement in HTTPADCExecutor. Writes temp sync files, records I/O metrics, builds validate args, issues per-backend validate requests (including APISIX POST /apisix/admin/configs/validate), parses responses into ADCValidateResult, and aggregates per-server validation errors. Includes APISIX payload transformation tests.
Validation error types
internal/types/error.go
Introduce ADCValidationErrors, ADCValidationError, ADCValidationServerAddrError, ADCValidationDetail with Error() implementations to represent and format aggregated validation failures.
Controller validation helpers
internal/controller/webhook_validation.go
Add Prepare*ForValidation helpers to build translation contexts for ApisixRoute, ApisixConsumer, v1alpha1.Consumer, and ApisixTls (ingress-class/gateway resolution and reconciler processing).
Webhook admission integration
internal/webhook/v1/adc_validation.go, internal/webhook/v1/apisixroute_webhook.go, internal/webhook/v1/apisixconsumer_webhook.go, internal/webhook/v1/apisixtls_webhook.go, internal/webhook/v1/consumer_webhook.go
Add adcAdmissionValidator that translates admission objects into ADC tasks and calls client.Validate. Webhooks initialize/store adcValidator and initErr; ValidateCreate/ValidateUpdate compute warnings and invoke ADC validation or return init error. Consumer webhook adds duplicate key-auth detection across Consumers sharing a gateway.
Webhook tests & helpers
internal/webhook/v1/..., internal/webhook/v1/adc_validation_test.go
Update test scheme registration and ingress-class seeding; add mock ADC server helpers; add tests for ADC rejection, fail-open behavior, APISIX validate endpoint usage, and consumer duplicate-key cases.
E2E webhook tests & helpers
test/e2e/webhook/*.go, test/e2e/webhook/helpers.go
Add expectAdmissionDenied helper; update e2e tests to assert admission denial for missing references or ADC validation failures; add ADC-specific e2e tests (invalid TLS/cert/plugin/credentials) and success-after-fix cases.
CI workflow
.github/workflows/e2e-test-k8s.yml
Change Kind kubeconfig acquisition to use kind export kubeconfig and ensure prior cluster teardown with make kind-down before make kind-up.

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)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
E2e Test Quality Review ⚠️ Warning E2E test suite has critical gaps: 4 failing tests for new ADC validation, no fail-open behavior tests, insufficient scenario coverage, generic assertions, and conditional test execution. Fix 4 failing e2e tests, add fail-open behavior tests with ADC unavailability simulation, enhance assertions for specific validation errors, add edge case scenarios, verify all resource types pass with apisix-standalone.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'feat: validate APISIX resources in webhooks' directly and clearly summarizes the main change: adding validation for APISIX resources through webhooks, which is the central theme across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Check ✅ Passed Comprehensive security review of ADC validation implementation across seven vulnerability categories found no critical security issues. Credentials are properly handled without logging or exposure in error messages.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/webhook-adc-validation

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

conformance test report - apisix-standalone mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T19:08:50Z"
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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

conformance test report - apisix mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T19:09:26Z"
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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T19:30:30Z"
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.

@AlinsRan AlinsRan marked this pull request as ready for review April 27, 2026 07:21
Copilot AI review requested due to automatic review settings April 27, 2026 07:21
Copy link
Copy Markdown
Contributor

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 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 adcAdmissionValidator to 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/validate support) 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

  • buildHTTPRequest logs the entire reqBody (including Token and full translated Config) 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.

Comment thread internal/adc/client/executor.go Outdated
Comment thread internal/adc/client/executor.go
Comment thread internal/webhook/v1/consumer_webhook.go
Comment thread internal/webhook/v1/consumer_webhook.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 | 🟡 Minor

Extract "apisix" to a constant to fix linter error.

The string "apisix" appears multiple times across test files. The linter flags this as a goconst violation.

🔧 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 making supportsEndpointSlice configurable.

The value supportsEndpointSlice: true is 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 true is 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.

validateDuplicateKeyAuthCredentials calls v.Client.List(ctx, &consumers) without any field/label selector, fetching all Consumer resources 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 setting MinVersion on 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.VersionTLS12 improves 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

📥 Commits

Reviewing files that changed from the base of the PR and between 15b2f39 and ba91ae8.

📒 Files selected for processing (20)
  • internal/adc/client/client.go
  • internal/adc/client/executor.go
  • internal/adc/client/executor_test.go
  • internal/controller/webhook_validation.go
  • internal/types/error.go
  • internal/webhook/v1/adc_validation.go
  • internal/webhook/v1/adc_validation_test.go
  • internal/webhook/v1/apisixconsumer_webhook.go
  • internal/webhook/v1/apisixconsumer_webhook_test.go
  • internal/webhook/v1/apisixroute_webhook.go
  • internal/webhook/v1/apisixroute_webhook_test.go
  • internal/webhook/v1/apisixtls_webhook.go
  • internal/webhook/v1/apisixtls_webhook_test.go
  • internal/webhook/v1/consumer_webhook.go
  • internal/webhook/v1/consumer_webhook_test.go
  • test/e2e/webhook/apisixconsumer.go
  • test/e2e/webhook/apisixroute.go
  • test/e2e/webhook/apisixtls.go
  • test/e2e/webhook/consumer.go
  • test/e2e/webhook/helpers.go

Comment thread internal/adc/client/executor.go
Comment thread internal/webhook/v1/adc_validation_test.go
Comment thread internal/webhook/v1/apisixconsumer_webhook.go
Comment thread internal/webhook/v1/apisixroute_webhook.go
Comment thread internal/webhook/v1/apisixtls_webhook.go
Comment thread internal/webhook/v1/consumer_webhook.go
Comment thread internal/webhook/v1/consumer_webhook.go
Comment thread test/e2e/webhook/apisixtls.go
AlinsRan and others added 2 commits April 27, 2026 15:41
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>
Copilot AI review requested due to automatic review settings April 27, 2026 07:46
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between ba91ae8 and 0b74a9c.

📒 Files selected for processing (4)
  • internal/adc/client/executor.go
  • internal/adc/client/executor_test.go
  • internal/webhook/v1/adc_validation_test.go
  • internal/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

Comment thread internal/adc/client/executor.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

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

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.

Comment thread internal/webhook/v1/consumer_webhook.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 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 explicit KUBECONFIG file 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

📥 Commits

Reviewing files that changed from the base of the PR and between 10d76b4 and d9ba66c.

📒 Files selected for processing (1)
  • .github/workflows/e2e-test-k8s.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 27, 2026 08:05
Copy link
Copy Markdown
Contributor

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

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.

Comment thread internal/controller/webhook_validation.go
Comment thread internal/adc/client/executor.go
Comment thread internal/webhook/v1/adc_validation.go
AlinsRan and others added 2 commits April 27, 2026 16:14
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 27, 2026 08:25
Copy link
Copy Markdown
Contributor

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

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.

AlinsRan and others added 2 commits April 27, 2026 16:49
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 27, 2026 16:48
Copy link
Copy Markdown
Contributor

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

Copilot reviewed 32 out of 32 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.

AlinsRan and others added 2 commits May 6, 2026 08:51
Revert CI/deployment changes that are not needed for the webhook ADC
validation feature and work fine on master:

- Revert docker login steps back to docker/login-action@v3 in all
  workflow files (apisix-conformance-test, apisix-e2e-test,
  conformance-test, e2e-test, e2e-test-k8s)
- Revert e2e-test-k8s.yml kind cluster setup to original approach
- Revert e2e-test-k8s.yml postgres image mirror preloading
- Revert spell-checker.yml to use wget-based misspell install
- Revert Makefile pull-infra-images to simple docker pull (keep
  jmalloc/echo-server removed since it is now built locally)
- Revert Makefile ADC binary download to simple curl | tar

Retained changes required for the feature:
- build-e2e-echo-server-image target
- kind-load-images dependency on build-e2e-echo-server-image

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 6, 2026 05:24
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ AlinsRan
❌ Foo Bar


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.

Copy link
Copy Markdown
Contributor

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

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Foo Bar and others added 2 commits May 6, 2026 13:35
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>
Copilot AI review requested due to automatic review settings May 6, 2026 05:55
Copy link
Copy Markdown
Contributor

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

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

Comment on lines +82 to +88
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
Comment on lines +75 to +78
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
Comment on lines +77 to +80
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
Comment on lines +78 to +81
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
AlinsRan and others added 2 commits May 6, 2026 14:59
- 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>
Copilot AI review requested due to automatic review settings May 6, 2026 07:10
…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>
Copy link
Copy Markdown
Contributor

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

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.

Comment on lines +82 to +88
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
}
Comment on lines +193 to +201
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)
Comment on lines +154 to +157

// 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
AlinsRan and others added 2 commits May 6, 2026 16:05
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>
Copilot AI review requested due to automatic review settings May 6, 2026 08:35
Copy link
Copy Markdown
Contributor

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

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

Comment on lines +82 to +87
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
Comment on lines +110 to +112
if v.initErr != nil || len(warnings) > 0 {
return warnings, nil
}
Comment on lines +75 to +80
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)
Comment on lines +93 to +98
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)
Comment on lines 45 to +49
// +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
Comment on lines 48 to +52
// +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
AlinsRan and others added 2 commits May 7, 2026 01:40
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>
Copilot AI review requested due to automatic review settings May 6, 2026 18:02
- 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>
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.

3 participants