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)
Comment |
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-27T17:02:49Z"
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. |
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-27T17:03:33Z"
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. |
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-27T17:26:29Z"
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.
| "url", validateURL, | ||
| "server", serverAddr, | ||
| "cacheKey", config.Name, | ||
| "body", body, |
There was a problem hiding this comment.
buildAPISIXValidateRequest logs the full validate payload (including TLS certificates/private keys and other sensitive config) via e.log.V(1). This can leak secrets into controller logs during admission validation. Consider removing the payload from logs or logging only a redacted/summary view (e.g., counts/IDs) and never the key material.
| "body", body, | |
| "payloadBytes", len(jsonData), |
| } | ||
| body.SSLs = append(body.SSLs, sslMap) | ||
| } | ||
|
|
There was a problem hiding this comment.
buildAPISIXValidatePayload defines fields for global_rules and plugin_metadata in apisixValidateRequest, but never populates them from resources.GlobalRules / resources.PluginMetadata. This can make standalone validation incomplete or inconsistent with what would be synced. Populate those fields (and any other relevant top-level resources) so validation matches actual config behavior.
| for _, globalRule := range resources.GlobalRules { | |
| if globalRule == nil { | |
| continue | |
| } | |
| globalRuleMap, err := toMap(globalRule) | |
| if err != nil { | |
| return nil, err | |
| } | |
| body.GlobalRules = append(body.GlobalRules, globalRuleMap) | |
| } | |
| for _, pluginMetadata := range resources.PluginMetadata { | |
| if pluginMetadata == nil { | |
| continue | |
| } | |
| pluginMetadataMap, err := toMap(pluginMetadata) | |
| if err != nil { | |
| return nil, err | |
| } | |
| body.PluginMetadata = append(body.PluginMetadata, pluginMetadataMap) | |
| } |
| var consumers apisixv1alpha1.ConsumerList | ||
| if err := v.Client.List(ctx, &consumers); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for i := range consumers.Items { | ||
| existing := &consumers.Items[i] | ||
| if existing.Namespace == consumer.Namespace && existing.Name == consumer.Name { | ||
| continue | ||
| } | ||
| if !sameConsumerGatewayRef(existing, consumer) { | ||
| continue | ||
| } | ||
|
|
||
| existingKeys, err := v.extractKeyAuthKeys(ctx, existing) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
validateDuplicateKeyAuthCredentials lists all Consumer objects on every create/update, and then may fetch referenced Secrets for each existing Consumer. This is O(N) (and potentially N+1 Secret GETs) in the admission path and can become a latency/availability problem in clusters with many Consumers. Consider scoping the list when gatewayRef.namespace is empty (in-namespace only), and/or using an indexed field selector / cached index keyed by gatewayRef + key-auth key to avoid full scans and repeated secret fetches.
| Key string `json:"key"` | ||
| } | ||
| if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { | ||
| return "", nil |
There was a problem hiding this comment.
extractCredentialKey silently ignores JSON unmarshal errors by returning "" and nil. If a key-auth credential has malformed JSON, this bypasses duplicate-key detection and may allow invalid config through (especially when ADC validation fails open). Return the unmarshal error (or convert it into an admission denial with a clear message) so malformed credential configs are rejected deterministically.
| return "", nil | |
| return "", fmt.Errorf("failed to parse key-auth credential config for consumer %s/%s: %w", consumer.Namespace, consumer.Name, err) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| type ApisixConsumerCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker | ||
| adcValidator *adcAdmissionValidator | ||
| initErr error | ||
| } | ||
|
|
||
| var _ webhook.CustomValidator = &ApisixConsumerCustomValidator{} | ||
|
|
||
| func NewApisixConsumerCustomValidator(c client.Client) *ApisixConsumerCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, apisixConsumerLog) | ||
| return &ApisixConsumerCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixConsumerLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixConsumerLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } |
There was a problem hiding this comment.
Same initErr concern applies here.
See comment on apisixroute_webhook.go – storing and returning initErr on every validation call means ADC validator initialization failures will permanently block all ApisixConsumer writes rather than failing open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/apisixconsumer_webhook.go` around lines 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.
| func NewApisixRouteCustomValidator(c client.Client) *ApisixRouteCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, apisixRouteLog) | ||
| return &ApisixRouteCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixRouteLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixRouteLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the best practice for Kubernetes admission webhook initialization errors - fail open or fail closed?
💡 Result:
The best practice for Kubernetes admission webhook initialization errors (timeouts, connection failures, or server errors) is to fail open for mutating webhooks (failurePolicy: Ignore) and fail closed for validating webhooks (failurePolicy: Fail, the default). For mutating webhooks, set failurePolicy to Ignore ("fail open") to prevent downtime from blocking compliant resource creation. Pair this with a separate validating webhook or controller to enforce policies on the final object state. This avoids disruptions from webhook unavailability while maintaining enforcement. For validating webhooks, use the default Fail ("fail closed") to ensure policy violations are blocked even during webhook issues, prioritizing security over availability. This distinction balances availability and security: mutations are best-effort and recoverable via validation, while validations are authoritative.
Citations:
- 1: https://kubernetes.io/docs/concepts/cluster-administration/admission-webhooks-good-practices/
- 2: https://k8s.ritovision.com/docs/concepts/cluster-administration/admission-webhooks-good-practices/
- 3: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/
- 4: https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/mutating-webhook-configuration-v1
🏁 Script executed:
# Determine webhook type and find related configuration
fd -t f "apisixroute_webhook" --exec cat {} \;Repository: api7/api7-ingress-controller
Length of output: 14040
🏁 Script executed:
# Check consumer_webhook.go for comparison of initialization pattern
fd -t f "consumer_webhook" --exec head -100 {} \;Repository: api7/api7-ingress-controller
Length of output: 14771
🏁 Script executed:
# Search for any WebhookConfiguration or failurePolicy references related to ApisixRoute
rg -A 5 -B 5 "ApisixRoute.*Webhook" --type goRepository: api7/api7-ingress-controller
Length of output: 2154
🏁 Script executed:
# Check ADC validator implementation to understand error handling semantics
fd -t f "*adc*" --type go | head -5Repository: api7/api7-ingress-controller
Length of output: 242
🏁 Script executed:
# Find ADC validator implementation
fd -i "*adc*" | grep -v test | grep -v __pycache__Repository: api7/api7-ingress-controller
Length of output: 357
🏁 Script executed:
# Search for newADCAdmissionValidator and its error conditions
rg -B 5 -A 20 "func newADCAdmissionValidator"Repository: api7/api7-ingress-controller
Length of output: 1941
🏁 Script executed:
# Check if there are any comments or documentation about fail-open strategy
rg -i "fail.open|fail.closed|fail.close" --type goRepository: api7/api7-ingress-controller
Length of output: 194
🏁 Script executed:
# Look for any tests that verify initErr behavior specifically
rg "initErr" --type go -B 3 -A 3Repository: api7/api7-ingress-controller
Length of output: 7835
🏁 Script executed:
# Search for any comments or documentation about initErr strategy
rg "initialization|init.*error|fail.*open" internal/webhook -i -B 2 -A 2Repository: api7/api7-ingress-controller
Length of output: 1732
🏁 Script executed:
# Check ADC client creation to understand failure modes
cat internal/webhook/v1/adc_validation.go | head -60Repository: api7/api7-ingress-controller
Length of output: 2567
🏁 Script executed:
# Verify the actual webhook configuration - check for any failurePolicy clarity
rg "kubebuilder:webhook" internal/webhook/v1/ -A 1Repository: api7/api7-ingress-controller
Length of output: 4551
🏁 Script executed:
# Check the Validate method in adcAdmissionValidator to see runtime error handling
rg -A 30 "func.*adcAdmissionValidator.*Validate" internal/webhook/v1/adc_validation.go | head -50Repository: api7/api7-ingress-controller
Length of output: 840
Initialization errors create inconsistent fail behavior compared to runtime ADC unavailability.
The adcAdmissionValidator.Validate() method logs ADC connection errors (500) and returns nil, allowing admission (fail-open). However, newADCAdmissionValidator() initialization errors are stored in initErr and returned immediately on every ValidateCreate/ValidateUpdate call, permanently denying all writes.
This creates two failure modes for the same ADC integration:
- ADC unavailable at validation time → admission succeeds ✓
- ADC client initialization fails → admission fails ✗
Either both should fail-open (return nil to allow admission during transient failures), or initialization errors should be distinguished as configuration errors in code and documentation. Currently, the behavior is inconsistent with the fail-open philosophy applied to runtime errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/apisixroute_webhook.go` around lines 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.
| warnings := v.collectWarnings(ctx, tls) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } | ||
|
|
||
| return warnings, v.adcValidator.Validate(ctx, tls) |
There was a problem hiding this comment.
Do not block admission on ADC validator init failures.
Returning initErr here turns ADC setup/backend problems into hard admission failures, which is the opposite of the PR’s fail-open contract. It also permanently poisons this validator until restart if initialization fails once.
🔧 Suggested fix
warnings := v.collectWarnings(ctx, tls)
if v.initErr != nil {
- return warnings, v.initErr
+ apisixTlsLog.Error(v.initErr, "ADC validation unavailable, allowing admission")
+ return warnings, nil
}
return warnings, v.adcValidator.Validate(ctx, tls)Also applies to: 106-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/apisixtls_webhook.go` around lines 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.
| type ConsumerCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker | ||
| adcValidator *adcAdmissionValidator | ||
| initErr error | ||
| } | ||
|
|
||
| var _ webhook.CustomValidator = &ConsumerCustomValidator{} | ||
|
|
||
| func NewConsumerCustomValidator(c client.Client) *ConsumerCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, consumerLog) | ||
| return &ConsumerCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, consumerLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, consumerLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } |
There was a problem hiding this comment.
Same initErr concern applies here.
See comment on apisixroute_webhook.go – storing and returning initErr causes permanent admission denial rather than fail-open behavior.
🤖 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 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.
| var cfg struct { | ||
| Key string `json:"key"` | ||
| } | ||
| if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { | ||
| return "", nil | ||
| } | ||
| return cfg.Key, nil |
There was a problem hiding this comment.
JSON unmarshal errors are silently ignored, potentially hiding malformed config.
If credential.Config.Raw contains invalid JSON, the error is swallowed and an empty key is returned. This could mask configuration errors from users.
Consider logging a warning or returning the error to surface malformed credential configs during validation.
🔧 Proposed fix to log malformed config
var cfg struct {
Key string `json:"key"`
}
if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil {
+ consumerLog.V(1).Info("failed to unmarshal key-auth config", "error", err, "consumer", consumer.Name)
return "", nil
}🤖 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 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.
| By("replacing the secret with valid certificate material") | ||
| err = s.DeleteResource("Secret", serverSecret) | ||
| Expect(err).NotTo(HaveOccurred(), "deleting invalid server TLS secret") | ||
|
|
||
| serverCert, serverKey := s.GenerateCert(GinkgoT(), []string{host}) | ||
| err = s.NewKubeTlsSecret(serverSecret, serverCert.String(), serverKey.String()) | ||
| Expect(err).NotTo(HaveOccurred(), "creating valid server TLS secret") | ||
|
|
||
| By("creating corrected ApisixTls") | ||
| err = s.CreateResourceFromString(tlsYAML) | ||
| Expect(err).NotTo(HaveOccurred(), "creating corrected ApisixTls") |
There was a problem hiding this comment.
Wait for the recreated Secret before retrying admission.
This path recreates serverSecret and immediately submits ApisixTls. The webhook resolves Secrets through a cached client, so this can race cache propagation and make the e2e intermittently fail on stale secret state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/webhook/apisixtls.go` around lines 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.
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
| 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.InsecureSkipVerify = true | ||
| } | ||
|
|
||
| return &http.Client{ | ||
| Timeout: e.httpClient.Timeout, | ||
| Transport: transport, | ||
| } | ||
| } |
There was a problem hiding this comment.
Set minimum TLS version to 1.2 or higher.
The TLS configuration lacks a MinVersion, which defaults to TLS 1.0 when acting as a client. This is a weak cryptographic configuration per CWE-327.
🔒 Proposed 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,
+ }
}
transport.TLSClientConfig.InsecureSkipVerify = true
+ } else {
+ if transport.TLSClientConfig == nil {
+ transport.TLSClientConfig = &tls.Config{}
+ }
+ transport.TLSClientConfig.MinVersion = tls.VersionTLS12
}
return &http.Client{As per coding guidelines, "TLS and cryptographic configuration must use correct semantics... default TLS min version to 1.2 or higher".
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 331-331: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 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 - 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.
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.
| var consumers apisixv1alpha1.ConsumerList | ||
| if err := v.Client.List(ctx, &consumers); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
validateDuplicateKeyAuthCredentials lists all Consumer objects cluster-wide and then iterates them to detect duplicates. On large clusters this makes admission latency O(total Consumers) (plus potential secret lookups), which can become a bottleneck. Consider adding a field index (e.g., by resolved GatewayRef) and using MatchingFields/scoped listing to narrow candidates before scanning.
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.
| reconciler := &ApisixRouteReconciler{ | ||
| Client: c, | ||
| Log: log, | ||
| ICGV: networkingv1.SchemeGroupVersion, | ||
| supportsEndpointSlice: true, | ||
| } |
There was a problem hiding this comment.
PrepareApisixRouteForValidation hardcodes supportsEndpointSlice: true, which can break validation on older clusters (e.g., k8s 1.18 where discovery.k8s.io/v1 EndpointSlice isn’t served). This can cause admission to error/deny routes because endpoint discovery fails. Consider detecting EndpointSlice support (similar to the reconciler’s HasAPIResource check) or defaulting to false/fallback to Endpoints for webhook validation.
| func buildAPISIXValidatePayload(resources *adctypes.Resources) (*apisixValidateRequest, error) { | ||
| body := &apisixValidateRequest{} | ||
|
|
||
| for _, service := range resources.Services { |
There was a problem hiding this comment.
buildAPISIXValidatePayload populates routes/services/consumers/ssls, but it never includes resources.GlobalRules or resources.PluginMetadata even though the request struct has global_rules / plugin_metadata fields. This means standalone validation can silently skip validating those resources. Consider converting these fields into the payload as well (or remove the unused fields if unsupported).
| configs, err := v.buildIngressClassConfigs(ctx, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(configs) == 0 { | ||
| return nil, nil | ||
| } | ||
| tctx, err = controller.PrepareApisixRouteForValidation(ctx, v.kubeClient, v.log, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result, err = v.translator.TranslateApisixRoute(tctx, resource.DeepCopy()) |
There was a problem hiding this comment.
In buildTask for v2 resources, ingress class parameters/configs are processed twice: first via buildIngressClassConfigs(...), then again inside controller.Prepare...ForValidation(...) (which calls FindMatchingIngressClassByObject + ProcessIngressClassParameters). This doubles API calls during admission. Consider reusing the TranslateContext from Prepare...ForValidation to build configs (and drop buildIngressClassConfigs), or have Prepare...ForValidation return configs directly.
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package main | ||
|
|
||
| import ( | ||
| "log" | ||
| "net/http" | ||
|
|
||
| "github.com/gorilla/websocket" | ||
| ) |
There was a problem hiding this comment.
New Go files under cmd/ appear to require the ASF license header (e.g., cmd/main.go). cmd/e2e-echo-server/main.go is missing this header; please add the standard Apache license block at the top for consistency/compliance.
| build-e2e-echo-server-image: | ||
| @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/e2e-echo-server ./cmd/e2e-echo-server | ||
| @docker build -f test/e2e/images/echo-server.Dockerfile -t jmalloc/echo-server:latest . |
There was a problem hiding this comment.
build-e2e-echo-server-image hard-codes GOARCH=amd64. This can break local dev and CI runners that are arm64 (or if KIND uses a non-amd64 node image). Consider deriving GOARCH from go env GOARCH / the kind node architecture, or using a multi-arch build strategy.
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 26 out of 26 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| var consumers apisixv1alpha1.ConsumerList | ||
| if err := v.Client.List(ctx, &consumers); err != nil { |
There was a problem hiding this comment.
validateDuplicateKeyAuthCredentials lists all Consumer objects and then filters in-memory. In clusters with many Consumers this makes admission (create/update) O(n) and can become a bottleneck. Since the codebase already field-indexes Consumers by gatewayRef (internal/controller/indexer/indexer.go:160-166,238-250), consider using a MatchingFields list scoped to the gatewayRef index key so only Consumers for the same gateway are scanned.
| if err := v.Client.List(ctx, &consumers); err != nil { | |
| if err := v.Client.List(ctx, &consumers, client.MatchingFields{ | |
| "gatewayRef": consumer.Spec.GatewayRef.Name, | |
| }); err != nil { |
| @@ -286,14 +290,18 @@ func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts met | |||
| } | |||
| return true, nil | |||
| } | |||
| return waitExponentialBackoff(condFunc) | |||
| return waitExponentialBackoffWithTimeout(condFunc, timeout) | |||
| } | |||
|
|
|||
| func waitExponentialBackoff(condFunc func() (bool, error)) error { | |||
| func waitExponentialBackoffWithTimeout(condFunc func() (bool, error), timeout time.Duration) error { | |||
| steps := int(timeout / (2 * time.Second)) | |||
| if steps < 1 { | |||
| steps = 1 | |||
| } | |||
| backoff := wait.Backoff{ | |||
| Duration: 500 * time.Millisecond, | |||
| Factor: 2, | |||
| Steps: 8, | |||
| Duration: 2 * time.Second, | |||
| Factor: 1, | |||
| Steps: steps, | |||
| } | |||
There was a problem hiding this comment.
WaitPodsAvailable now hardcodes a 1 minute timeout and the underlying backoff is effectively constant 2s polling (Factor=1). Previously this helper waited ~2+ minutes (500ms * 2^Steps) and is used in many e2e deploy paths; reducing the default timeout can introduce flakes for slower clusters. Consider restoring a longer default (or keeping the old backoff parameters) and using WaitPodsAvailableWithTimeout only when a shorter timeout is explicitly desired.
| case *apiv2.ApisixRoute: | ||
| configs, err := v.buildIngressClassConfigs(ctx, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(configs) == 0 { | ||
| return nil, nil | ||
| } | ||
| tctx, err = controller.PrepareApisixRouteForValidation(ctx, v.kubeClient, v.log, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result, err = v.translator.TranslateApisixRoute(tctx, resource.DeepCopy()) | ||
| resourceTypes = append(resourceTypes, adctypes.TypeService) |
There was a problem hiding this comment.
buildTask() does ingress-class resolution twice for ApisixRoute/ApisixConsumer/ApisixTls: buildIngressClassConfigs() calls FindMatchingIngressClassByObject + ProcessIngressClassParameters, and then PrepareForValidation does the same again. This duplicates client/cache lookups on every admission request and is avoidable. Consider calling PrepareForValidation first and deriving configs via v.buildConfigs(tctx), or refactoring so ingress-class processing happens only once per Validate().
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 31 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, getErr := s.GetOutputFromString(resourceType, resourceName, "-o", "yaml") | ||
| Expect(getErr).To(HaveOccurred(), fmt.Sprintf("resource %s/%s should not exist after admission rejection", resourceType, resourceName)) |
There was a problem hiding this comment.
expectAdmissionDenied verifies that a follow-up kubectl get fails, but it doesn't assert the failure is specifically a NotFound. As written, the test could pass even if kubectl get failed for an unrelated reason (e.g., bad resourceType, transient API error). Tighten the assertion to check the error/output indicates the resource was not found (or use a typed NotFound check) so admission-deny tests can't produce false positives.
| Factor: 2, | ||
| Steps: 8, | ||
| Duration: 2 * time.Second, | ||
| Factor: 1, |
There was a problem hiding this comment.
waitExponentialBackoffWithTimeout uses wait.ExponentialBackoff with Factor=1, which results in a constant polling interval rather than exponential backoff. Either adjust the backoff parameters to actually be exponential, or rename the helper to avoid implying exponential behavior.
| Factor: 1, | |
| Factor: 2, |
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 31 out of 31 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configs, err := v.buildIngressClassConfigs(ctx, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(configs) == 0 { | ||
| return nil, nil | ||
| } | ||
| tctx, err = controller.PrepareApisixRouteForValidation(ctx, v.kubeClient, v.log, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result, err = v.translator.TranslateApisixRoute(tctx, resource.DeepCopy()) | ||
| resourceTypes = append(resourceTypes, adctypes.TypeService) | ||
| if err != nil { |
There was a problem hiding this comment.
buildTask calls buildIngressClassConfigs() (which runs FindMatchingIngressClassByObject + ProcessIngressClassParameters) and then calls PrepareApisix*ForValidation() which repeats the same ingress class matching/parameter processing. This duplicates API reads and makes admission slower than necessary. Consider building configs from the TranslateContext returned by the Prepare...ForValidation helpers instead of re-processing the IngressClass twice.
| package main | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This new Go file is missing the standard ASF license header that other cmd/ entrypoints in this repo include. Add the Apache Software Foundation header comment at the top for license compliance/consistency.
| @@ -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 | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +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 | |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.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 | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +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 | |
| // +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 |
| @@ -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 | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +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 | |
| // +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 |
| @@ -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 | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +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-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 |
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 31 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) | ||
| continue | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Client.Validate returns immediately on the first non-ADCValidationError from executor.Validate. This can cause a mixed outcome to fail-open: if one GatewayProxy config has an infra error (e.g., backend unavailable) but another config would return explicit validation errors, the infra error will short-circuit and the admission layer will treat it as an infra error and allow the request. Consider aggregating infra errors across configs and only returning them if no validation errors were found for any config (similar to how per-server validation is aggregated in runHTTPValidate).
|
|
||
| "github.com/gorilla/websocket" | ||
| ) | ||
|
|
||
| var upgrader = websocket.Upgrader{ | ||
| CheckOrigin: func(*http.Request) bool { | ||
| return true | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
websocket.Upgrader is configured with CheckOrigin always returning true, which disables the origin check entirely. Even for an e2e helper, this pattern is risky if the binary is ever reused outside tests; consider restricting origins (e.g., same-host) or making this explicitly configurable/documented as test-only behavior.
| "github.com/gorilla/websocket" | |
| ) | |
| var upgrader = websocket.Upgrader{ | |
| CheckOrigin: func(*http.Request) bool { | |
| return true | |
| }, | |
| } | |
| "net/url" | |
| "github.com/gorilla/websocket" | |
| ) | |
| func sameHostOrigin(r *http.Request) bool { | |
| origin := r.Header.Get("Origin") | |
| if origin == "" { | |
| return true | |
| } | |
| u, err := url.Parse(origin) | |
| if err != nil { | |
| return false | |
| } | |
| return u.Host == r.Host | |
| } | |
| var upgrader = websocket.Upgrader{ | |
| CheckOrigin: sameHostOrigin, | |
| } |
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | ||
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, time.Minute) |
There was a problem hiding this comment.
WaitPodsAvailable now hard-codes a 1 minute timeout, whereas the previous exponential backoff (500ms * 2^n, 8 steps) effectively waited ~2 minutes. This change can make e2e deployments flakier in slow CI clusters. Consider keeping the previous default (or increasing it) and using WaitPodsAvailableWithTimeout only at call sites that need custom bounds.
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | |
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, time.Minute) | |
| const waitPodsAvailableDefaultTimeout = 127500 * time.Millisecond | |
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | |
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, waitPodsAvailableDefaultTimeout) |
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 31 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| .PHONY: build-e2e-echo-server-image | ||
| build-e2e-echo-server-image: | ||
| @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/e2e-echo-server ./cmd/e2e-echo-server |
There was a problem hiding this comment.
build-e2e-echo-server-image hard-codes GOARCH=amd64. This will break running e2e tests on arm64 hosts/clusters (the scratch image will contain an amd64 binary). Consider using the existing $(GOARCH) Makefile variable (or building/loading a multi-arch image) so the echo-server image matches the target platform.
| @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/e2e-echo-server ./cmd/e2e-echo-server | |
| @CGO_ENABLED=0 GOOS=linux GOARCH=$(GOARCH) go build -o bin/e2e-echo-server ./cmd/e2e-echo-server |
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), `duplicate key-auth credential key "shared-key"`) | ||
| require.Contains(t, err.Error(), "default/existing") | ||
| } |
There was a problem hiding this comment.
The new duplicate key-auth detection test only covers the inline credential.Config.Raw path. Since the implementation also supports extracting the key from credential.SecretRef (and has special handling for missing/invalid data), consider adding unit coverage for the SecretRef-based key extraction to ensure duplicate detection works for the common Secret-backed credential flow.
| } | |
| } | |
| func TestConsumerValidator_DenyDuplicateKeyAuthCredentialFromSecretRef(t *testing.T) { | |
| existing := &apisixv1alpha1.Consumer{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "existing", | |
| Namespace: "default", | |
| }, | |
| Spec: apisixv1alpha1.ConsumerSpec{ | |
| GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, | |
| Credentials: []apisixv1alpha1.Credential{{ | |
| Type: "key-auth", | |
| SecretRef: &apisixv1alpha1.SecretReference{ | |
| Name: "existing-key-secret", | |
| }, | |
| }}, | |
| }, | |
| } | |
| consumer := &apisixv1alpha1.Consumer{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "demo", | |
| Namespace: "default", | |
| }, | |
| Spec: apisixv1alpha1.ConsumerSpec{ | |
| GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, | |
| Credentials: []apisixv1alpha1.Credential{{ | |
| Type: "key-auth", | |
| SecretRef: &apisixv1alpha1.SecretReference{ | |
| Name: "demo-key-secret", | |
| }, | |
| }}, | |
| }, | |
| } | |
| validator := buildConsumerValidator( | |
| t, | |
| existing, | |
| &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "existing-key-secret", Namespace: "default"}, | |
| Data: map[string][]byte{ | |
| "key": []byte("shared-key"), | |
| }, | |
| }, | |
| &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "demo-key-secret", Namespace: "default"}, | |
| Data: map[string][]byte{ | |
| "key": []byte("shared-key"), | |
| }, | |
| }, | |
| ) | |
| warnings, err := validator.ValidateCreate(context.Background(), consumer) | |
| require.Empty(t, warnings) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), `duplicate key-auth credential key "shared-key"`) | |
| require.Contains(t, err.Error(), "default/existing") | |
| } |
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 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.
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