Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis change renames the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ADC upstream active health check payload to use the standalone schema’s certificate verification field name, ensuring generated configs are accepted by the standalone dataplane and health checks can sync successfully.
Changes:
- Renamed the active health check verify field in the ADC payload from
https_verify_certtohttps_verify_certificate. - Updated the upstream translator to set
HTTPSVerifyCertificatebased onActiveHealthCheck.StrictTLS. - Added unit test coverage for the default (strict TLS enabled) and explicit strict TLS disabled cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/adc/translator/apisixupstream.go |
Switches translator output from HTTPSVerifyCert to HTTPSVerifyCertificate when strict TLS is enabled/defaulted. |
api/adc/types.go |
Renames the ADC active health check field/tag to https_verify_certificate to match updated standalone schema. |
internal/adc/translator/apisixupstream_test.go |
Adds focused unit tests verifying default enablement and explicit disablement behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/adc/types.go`:
- Line 207: The HTTPSVerifyCertificate field in the ADC config struct is
currently a plain bool with `omitempty`, which causes explicit false to be
dropped and APISIX to default to true; change the field declaration for
HTTPSVerifyCertificate to be a pointer (*bool) so false is preserved (or
alternatively remove `omitempty` from its `json`/`yaml` tags) and update any
constructors or JSON/YAML marshalling assumptions that set or read
HTTPSVerifyCertificate to handle a nil vs. non-nil value (look for the
struct/type that declares HTTPSVerifyCertificate and any code paths that build
or serialize that struct).
🪄 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: 75aa56bc-9d4c-4dfe-b33e-ac1cab95bef3
📒 Files selected for processing (3)
api/adc/types.gointernal/adc/translator/apisixupstream.gointernal/adc/translator/apisixupstream_test.go
| Host string `json:"host,omitempty" yaml:"host,omitempty"` | ||
| Port int32 `json:"port,omitempty" yaml:"port,omitempty"` | ||
| HTTPPath string `json:"http_path,omitempty" yaml:"http_path,omitempty"` | ||
| HTTPSVerifyCertificate bool `json:"https_verify_certificate,omitempty" yaml:"https_verify_certificate,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's encoding/json and common YAML marshalling, does omitemptyomit aboolfield when its value isfalse? Also, for APISIX upstream active health checks, what is the default behavior of https_verify_certificate when the field is omitted?
💡 Result:
- Go encoding/json + common YAML marshalling: does omitempty omit a bool when it is false? Yes for Go’s encoding/json. In Go’s encoding/json, a struct field tagged with omitempty is omitted when the field has an “empty value”; for bool, that empty value is false. 2) For APISIX upstream active health checks: default behavior of https_verify_certificate when omitted When using APISIX active health checks with HTTPS probes, the field upstream.checks.active.https_verify_certificate defaults to true (i.e., certificate verification is performed).
Citations:
- 1: https://pkg.go.dev/encoding/json@go1.25.5
- 2: https://pkg.go.dev/encoding/json
- 3: https://apisix.apache.org/docs/apisix/next/tutorials/health-check
- 4: https://apisix.apache.org/docs/apisix/next/tutorials/health-check/
Fix the TLS verification field to prevent silent inversion of boolean state.
The HTTPSVerifyCertificate field is a plain bool with omitempty. When StrictTLS: false (intending to disable certificate verification), the field is set to false, which then gets omitted from the serialized output due to omitempty. APISIX defaults the missing https_verify_certificate field to true, inverting the intended behavior—users disabling verification end up with verification enabled instead.
Change this to *bool to preserve the explicit false state, or remove the omitempty tag to always emit the field. This is a security-relevant configuration issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/adc/types.go` at line 207, The HTTPSVerifyCertificate field in the ADC
config struct is currently a plain bool with `omitempty`, which causes explicit
false to be dropped and APISIX to default to true; change the field declaration
for HTTPSVerifyCertificate to be a pointer (*bool) so false is preserved (or
alternatively remove `omitempty` from its `json`/`yaml` tags) and update any
constructors or JSON/YAML marshalling assumptions that set or read
HTTPSVerifyCertificate to handle a nil vs. non-nil value (look for the
struct/type that declares HTTPSVerifyCertificate and any code paths that build
or serialize that struct).
Contributor
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-30T06:54: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:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-30T06:54:35Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips.
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
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. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ADC upstream active health-check TLS verification field naming to match the standalone dataplane schema so generated configs are accepted and upstreams sync correctly.
Changes:
- Rename the active health-check verify field in the ADC payload to
https_verify_certificate(Go fieldHTTPSVerifyCertificate). - Update upstream translation logic to set the new field when
StrictTLSis enabled/defaulted. - Set
ADC_VERSION: devin API7EE e2e workflows so the CI path that extracts/uses the dev ADC artifact is enabled.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/adc/translator/apisixupstream.go |
Switches active health-check TLS verify output to the new HTTPSVerifyCertificate field. |
api/adc/types.go |
Renames the serialized JSON/YAML field from https_verify_cert to https_verify_certificate for active health checks. |
.github/workflows/e2e-test.yml |
Sets ADC_VERSION: dev globally for the workflow. |
.github/workflows/e2e-test-k8s.yml |
Sets ADC_VERSION: dev globally for the workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
323
to
327
| active.HTTPRequestHeaders = config.RequestHeaders | ||
|
|
||
| if config.StrictTLS == nil || *config.StrictTLS { | ||
| active.HTTPSVerifyCert = true | ||
| active.HTTPSVerifyCertificate = true | ||
| } |
Comment on lines
+31
to
+33
| env: | ||
| ADC_VERSION: dev | ||
|
|
Comment on lines
+31
to
+33
| env: | ||
| ADC_VERSION: dev | ||
|
|
Contributor
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-04-30T07:15:48Z"
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:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures. |
Baoyuantop
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
https_verify_certificateWhy
The standalone dataplane rejects
https_verify_certin active health checks, which prevents the generated service config from syncing and leaves the upstream list empty in the affected e2e case.Summary by CodeRabbit