Copilot commented on code in PR #2703:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2703#discussion_r2918680075
##########
test/e2e/scaffold/scaffold.go:
##########
@@ -313,6 +313,46 @@ func (s *Scaffold) NewAPISIXClientWithTLSProxy(host
string) *httpexpect.Expect {
})
}
+// NewAPISIXClientForPort creates an HTTP client for a specific APISIX port.
+// Uses existing tunnels if available, otherwise creates a new one.
+func (s *Scaffold) NewAPISIXClientForPort(port int) (*httpexpect.Expect,
error) {
+ // Check if we can reuse existing tunnels
+ switch port {
+ case 80:
+ return s.NewAPISIXClient(), nil
+ case 443:
+ return s.NewAPISIXHttpsClient(""), nil
+ case 9100:
+ return s.NewAPISIXClientOnTCPPort(), nil
+ }
+
+ // Create new tunnel for custom port
+ serviceName := s.dataplaneService.Name
+ tunnel := k8s.NewTunnel(s.kubectlOptions, k8s.ResourceTypeService,
serviceName, 0, port)
+ if err := tunnel.ForwardPortE(s.t); err != nil {
+ return nil, fmt.Errorf("failed to create tunnel for port %d:
%w", port, err)
+ }
+ s.addFinalizers(tunnel.Close)
+
+ u := url.URL{
+ Scheme: "http",
+ Host: tunnel.Endpoint(),
+ }
+ return httpexpect.WithConfig(httpexpect.Config{
+ BaseURL: u.String(),
+ Client: &http.Client{
+ Transport: &http.Transport{TLSClientConfig:
&tls.Config{InsecureSkipVerify: true}},
+ Timeout: 3 * time.Second,
+ CheckRedirect: func(req *http.Request, via
[]*http.Request) error {
+ return http.ErrUseLastResponse
+ },
+ },
+ Reporter: httpexpect.NewAssertReporter(
+ httpexpect.NewAssertReporter(GinkgoT()),
+ ),
Review Comment:
The reporter is wrapped with `httpexpect.NewAssertReporter` twice. The inner
call returns a Reporter, not a `testing.T`-like value, so this is likely a
compile-time type error (or at best an unintended reporter chain). Use a single
`httpexpect.NewAssertReporter(GinkgoT())` here.
```suggestion
Reporter: httpexpect.NewAssertReporter(GinkgoT()),
```
##########
test/e2e/scaffold/k8s.go:
##########
@@ -268,6 +268,28 @@ func (s *Scaffold) RunDigDNSClientFromK8s(args ...string)
(string, error) {
return s.RunKubectlAndGetOutput(kubectlArgs...)
}
+// RunCurlFromK8s runs a curl command from a temporary pod inside the cluster.
+// This is useful for making HTTP requests from within the cluster, avoiding
+// port-forward limitations where server_port variables may not work correctly.
+func (s *Scaffold) RunCurlFromK8s(args ...string) (string, error) {
+ podName := fmt.Sprintf("curl-test-%d", time.Now().UnixNano())
+ kubectlArgs := []string{
+ "run",
+ podName,
+ "--attach=true",
+ "--rm",
+ "--restart=Never",
+ "--image-pull-policy=IfNotPresent",
+ "--image=alpine/curl:latest",
Review Comment:
Using a `:latest` tag makes E2E runs non-reproducible and can break
unexpectedly when the upstream image changes. Please pin this to a specific
version (or digest) and keep the Makefile kind-load/pull targets in sync with
that pinned reference.
```suggestion
"--image=alpine/curl:8.10.1",
```
##########
test/e2e/scaffold/scaffold.go:
##########
@@ -313,6 +313,46 @@ func (s *Scaffold) NewAPISIXClientWithTLSProxy(host
string) *httpexpect.Expect {
})
}
+// NewAPISIXClientForPort creates an HTTP client for a specific APISIX port.
+// Uses existing tunnels if available, otherwise creates a new one.
Review Comment:
The docstring says it 'uses existing tunnels if available, otherwise creates
a new one', but custom ports always create a brand-new tunnel and there’s no
caching/reuse on subsequent calls. Either adjust the comment to reflect the
actual behavior, or add a simple per-port tunnel cache on the Scaffold to avoid
repeated port-forward setup in tests that call this helper multiple times.
```suggestion
// For well-known ports (80, 443, 9100), it reuses existing tunnels; for any
// other port, it always creates a new tunnel and does not cache or reuse it.
```
##########
Makefile:
##########
@@ -204,6 +205,7 @@ pull-infra-images:
@docker pull kennethreitz/httpbin:latest
@docker pull jmalloc/echo-server:latest
@docker pull openresty/openresty:1.27.1.2-4-bullseye-fat
+ @docker pull alpine/curl:latest
Review Comment:
For the same reproducibility reasons as the E2E kubectl-run image,
`alpine/curl:latest` should be pinned to a version (or digest). This will
reduce flakes due to upstream changes and make CI failures easier to diagnose.
##########
internal/controller/utils.go:
##########
@@ -362,6 +362,10 @@ func ParseRouteParentRefs(
reason := gatewayv1.RouteReasonNoMatchingParent
var listenerName string
var matchedListener gatewayv1.Listener
+ var matchedListeners []gatewayv1.Listener
+
+ // Track if sectionName was explicitly specified
+ sectionNameSpecified := parentRef.SectionName != nil &&
*parentRef.SectionName != ""
for _, listener := range gateway.Spec.Listeners {
if parentRef.SectionName != nil {
Review Comment:
The loop still treats `parentRef.SectionName != nil` as an active
sectionName constraint, even if it is an empty string. That contradicts
`sectionNameSpecified` and can cause routes to fail matching any listener when
a (possibly user-generated) empty sectionName is present. Consider gating the
sectionName-specific matching block on `sectionNameSpecified` instead of
`parentRef.SectionName != nil`.
##########
Makefile:
##########
@@ -185,9 +185,10 @@ kind-down:
.PHONY: kind-load-images
kind-load-images: pull-infra-images kind-load-ingress-image kind-load-adc-image
- @kind load docker-image kennethreitz/httpbin:latest --name $(KIND_NAME)
+ @kind load docker-image kennethreitz/httpbin:latest --name $(KIND_NAME)
@kind load docker-image jmalloc/echo-server:latest --name $(KIND_NAME)
@kind load docker-image openresty/openresty:1.27.1.2-4-bullseye-fat
--name $(KIND_NAME)
+ @kind load docker-image alpine/curl:latest --name $(KIND_NAME)
Review Comment:
For the same reproducibility reasons as the E2E kubectl-run image,
`alpine/curl:latest` should be pinned to a version (or digest). This will
reduce flakes due to upstream changes and make CI failures easier to diagnose.
##########
test/e2e/scaffold/scaffold.go:
##########
@@ -313,6 +313,46 @@ func (s *Scaffold) NewAPISIXClientWithTLSProxy(host
string) *httpexpect.Expect {
})
}
+// NewAPISIXClientForPort creates an HTTP client for a specific APISIX port.
+// Uses existing tunnels if available, otherwise creates a new one.
+func (s *Scaffold) NewAPISIXClientForPort(port int) (*httpexpect.Expect,
error) {
+ // Check if we can reuse existing tunnels
+ switch port {
+ case 80:
+ return s.NewAPISIXClient(), nil
+ case 443:
+ return s.NewAPISIXHttpsClient(""), nil
+ case 9100:
+ return s.NewAPISIXClientOnTCPPort(), nil
+ }
+
+ // Create new tunnel for custom port
+ serviceName := s.dataplaneService.Name
+ tunnel := k8s.NewTunnel(s.kubectlOptions, k8s.ResourceTypeService,
serviceName, 0, port)
Review Comment:
The docstring says it 'uses existing tunnels if available, otherwise creates
a new one', but custom ports always create a brand-new tunnel and there’s no
caching/reuse on subsequent calls. Either adjust the comment to reflect the
actual behavior, or add a simple per-port tunnel cache on the Scaffold to avoid
repeated port-forward setup in tests that call this helper multiple times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
