Copilot commented on code in PR #2551:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2551#discussion_r2335197670
##########
test/e2e/scaffold/scaffold.go:
##########
@@ -63,23 +63,41 @@ type Scaffold struct {
dataplaneService *corev1.Service
httpbinService *corev1.Service
- finalizers []func()
-
- apisixHttpTunnel *k8s.Tunnel
- apisixHttpsTunnel *k8s.Tunnel
+ finalizers []func()
+ apisixTunnels *Tunnels
additionalGateways map[string]*GatewayResources
runtimeOpts Options
Deployer Deployer
}
+type Tunnels struct {
+ HTTP *k8s.Tunnel
+ HTTPS *k8s.Tunnel
+ TCP *k8s.Tunnel
+}
+
+func (t *Tunnels) Close() {
+ if t.HTTP != nil {
+ t.HTTP.Close()
+ t.HTTP = nil
+ }
+ if t.HTTPS != nil {
+ t.HTTPS.Close()
+ t.HTTPS = nil
+ }
+ if t.TCP != nil {
+ t.TCP.Close()
+ t.TCP = nil
+ }
+}
Review Comment:
[nitpick] The Close method should handle panic scenarios. Consider using
defer or recover to ensure all tunnels are closed even if one Close() call
panics.
##########
test/e2e/crds/v2/streamroute.go:
##########
@@ -0,0 +1,222 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package v2
+
+import (
+ "fmt"
+ "time"
+
+ mqtt "github.com/eclipse/paho.mqtt.golang"
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
+
+ "github.com/apache/apisix-ingress-controller/test/e2e/framework"
+ "github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
+)
+
+var _ = Describe("Test ApisixRoute With StreamRoute",
Label("apisix.apache.org", "v2", "apisixroute"), func() {
+ s := scaffold.NewDefaultScaffold()
+
+ BeforeEach(func() {
+ if framework.ProviderType != framework.ProviderTypeAPISIX {
+ Skip("only support APISIX provider")
+ }
+ By("create GatewayProxy")
+ gatewayProxy := s.GetGatewayProxyYaml()
+ err := s.CreateResourceFromString(gatewayProxy)
+ Expect(err).NotTo(HaveOccurred(), "creating GatewayProxy")
+ time.Sleep(5 * time.Second)
+
+ By("create IngressClass")
+ err =
s.CreateResourceFromStringWithNamespace(s.GetIngressClassYaml(), "")
+ Expect(err).NotTo(HaveOccurred(), "creating IngressClass")
+ time.Sleep(5 * time.Second)
+ })
+
+ Context("TCP Proxy", func() {
+ apisixRoute := `
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: httpbin-tcp-route
+spec:
+ ingressClassName: %s
+ stream:
+ - name: rule1
+ protocol: TCP
+ match:
+ ingressPort: 9100
+ backend:
+ serviceName: httpbin-service-e2e-test
+ servicePort: 80
+`
+ It("stream tcp proxy", func() {
+ err :=
s.CreateResourceFromString(fmt.Sprintf(apisixRoute, s.Namespace()))
+ Expect(err).NotTo(HaveOccurred(), "creating
ApisixRoute")
+
+ s.RequestAssert(&scaffold.RequestAssert{
+ Client: s.NewAPISIXClientWithTCPProxy(),
+ Method: "GET",
+ Path: "/ip",
+ Checks: []scaffold.ResponseCheckFunc{
+ scaffold.WithExpectedStatus(200),
+
scaffold.WithExpectedBodyContains("origin"),
+ },
+ })
+
+ s.RequestAssert(&scaffold.RequestAssert{
+ Client: s.NewAPISIXClientWithTCPProxy(),
+ Method: "GET",
+ Path: "/get",
+ Headers: map[string]string{
+ "x-my-header": "x-my-value",
+ },
+ Checks: []scaffold.ResponseCheckFunc{
+ scaffold.WithExpectedStatus(200),
+
scaffold.WithExpectedBodyContains("x-my-value"),
+ },
+ })
+ })
+ })
+
+ Context("UDP Proxy", func() {
+ apisixRoute := `
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: httpbin-udp-route
+spec:
+ ingressClassName: %s
+ stream:
+ - name: rule1
+ protocol: UDP
+ match:
+ ingressPort: 9200
+ backend:
+ serviceName: %s
+ servicePort: %d
+`
+ It("stream udp proxy", func() {
+ dnsSvc := s.NewCoreDNSService()
+ err :=
s.CreateResourceFromString(fmt.Sprintf(apisixRoute, s.Namespace(), dnsSvc.Name,
dnsSvc.Spec.Ports[0].Port))
+ Expect(err).NotTo(HaveOccurred(), "creating
ApisixRoute")
+ time.Sleep(20 * time.Second)
+
+ svc := s.GetDataplaneService()
+
+ // test dns query
+ output, err :=
s.RunDigDNSClientFromK8s(fmt.Sprintf("@%s", svc.Name), "-p", "9200",
"github.com")
Review Comment:
[nitpick] Consider using a constant for the UDP port number (9200) instead
of hardcoding it, since it's also used in the APISIX configuration and route
definition.
##########
test/e2e/crds/v2/streamroute.go:
##########
@@ -0,0 +1,222 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package v2
+
+import (
+ "fmt"
+ "time"
+
+ mqtt "github.com/eclipse/paho.mqtt.golang"
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
+
+ "github.com/apache/apisix-ingress-controller/test/e2e/framework"
+ "github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
+)
+
+var _ = Describe("Test ApisixRoute With StreamRoute",
Label("apisix.apache.org", "v2", "apisixroute"), func() {
+ s := scaffold.NewDefaultScaffold()
+
+ BeforeEach(func() {
+ if framework.ProviderType != framework.ProviderTypeAPISIX {
+ Skip("only support APISIX provider")
+ }
+ By("create GatewayProxy")
+ gatewayProxy := s.GetGatewayProxyYaml()
+ err := s.CreateResourceFromString(gatewayProxy)
+ Expect(err).NotTo(HaveOccurred(), "creating GatewayProxy")
+ time.Sleep(5 * time.Second)
+
+ By("create IngressClass")
+ err =
s.CreateResourceFromStringWithNamespace(s.GetIngressClassYaml(), "")
+ Expect(err).NotTo(HaveOccurred(), "creating IngressClass")
+ time.Sleep(5 * time.Second)
+ })
+
+ Context("TCP Proxy", func() {
+ apisixRoute := `
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: httpbin-tcp-route
+spec:
+ ingressClassName: %s
+ stream:
+ - name: rule1
+ protocol: TCP
+ match:
+ ingressPort: 9100
+ backend:
+ serviceName: httpbin-service-e2e-test
+ servicePort: 80
+`
+ It("stream tcp proxy", func() {
+ err :=
s.CreateResourceFromString(fmt.Sprintf(apisixRoute, s.Namespace()))
+ Expect(err).NotTo(HaveOccurred(), "creating
ApisixRoute")
+
+ s.RequestAssert(&scaffold.RequestAssert{
+ Client: s.NewAPISIXClientWithTCPProxy(),
+ Method: "GET",
+ Path: "/ip",
+ Checks: []scaffold.ResponseCheckFunc{
+ scaffold.WithExpectedStatus(200),
+
scaffold.WithExpectedBodyContains("origin"),
+ },
+ })
+
+ s.RequestAssert(&scaffold.RequestAssert{
+ Client: s.NewAPISIXClientWithTCPProxy(),
+ Method: "GET",
+ Path: "/get",
+ Headers: map[string]string{
+ "x-my-header": "x-my-value",
+ },
+ Checks: []scaffold.ResponseCheckFunc{
+ scaffold.WithExpectedStatus(200),
+
scaffold.WithExpectedBodyContains("x-my-value"),
+ },
+ })
+ })
+ })
+
+ Context("UDP Proxy", func() {
+ apisixRoute := `
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: httpbin-udp-route
+spec:
+ ingressClassName: %s
+ stream:
+ - name: rule1
+ protocol: UDP
+ match:
+ ingressPort: 9200
+ backend:
+ serviceName: %s
+ servicePort: %d
+`
+ It("stream udp proxy", func() {
+ dnsSvc := s.NewCoreDNSService()
+ err :=
s.CreateResourceFromString(fmt.Sprintf(apisixRoute, s.Namespace(), dnsSvc.Name,
dnsSvc.Spec.Ports[0].Port))
+ Expect(err).NotTo(HaveOccurred(), "creating
ApisixRoute")
+ time.Sleep(20 * time.Second)
+
+ svc := s.GetDataplaneService()
+
+ // test dns query
+ output, err :=
s.RunDigDNSClientFromK8s(fmt.Sprintf("@%s", svc.Name), "-p", "9200",
"github.com")
+ Expect(err).NotTo(HaveOccurred(), "dig github.com via
apisix udp proxy")
+ Expect(output).To(ContainSubstring("ADDITIONAL
SECTION"))
+
+ time.Sleep(3 * time.Second)
+ output = s.GetDeploymentLogs(scaffold.CoreDNSDeployment)
+ Expect(output).To(ContainSubstring("github.com. udp"))
+ })
+ })
+
+ Context("Plugins", func() {
+ It("MQTT", func() {
+ //nolint:misspell // eclipse-mosquitto is the correct
image name
+ mqttDeploy := `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: mosquito
Review Comment:
The deployment name should be 'mosquitto' (with double 't') to match the
correct spelling of the Eclipse Mosquitto MQTT broker.
##########
internal/adc/translator/apisixroute.go:
##########
@@ -341,6 +349,20 @@ func (t *Translator)
translateApisixRouteBackendResolveGranularityService(tctx *
}, nil
}
+func (t *Translator) translateApisixRouteStreamBackendResolveGranularity(tctx
*provider.TranslateContext, arNN types.NamespacedName, backend
apiv2.ApisixRouteStreamBackend) (adc.UpstreamNodes, error) {
+ tsBakcnd := apiv2.ApisixRouteHTTPBackend{
Review Comment:
Variable name 'tsBakcnd' contains a typo. It should be 'tsBackend' for
better readability.
##########
api/adc/types.go:
##########
@@ -195,15 +195,14 @@ type Timeout struct {
// +k8s:deepcopy-gen=true
type StreamRoute struct {
- Description string `json:"description,omitempty"`
- ID string `json:"id,omitempty"`
- Labels map[string]string `json:"labels,omitempty"`
- Name string `json:"name"`
- Plugins Plugins `json:"plugins,omitempty"`
- RemoteAddr string `json:"remote_addr,omitempty"`
- ServerAddr string `json:"server_addr,omitempty"`
- ServerPort *int64 `json:"server_port,omitempty"`
- Sni string `json:"sni,omitempty"`
+ Metadata `json:",inline" yaml:",inline"`
+
+ Labels map[string]string `json:"labels,omitempty"`
+ Plugins Plugins `json:"plugins,omitempty"`
+ RemoteAddr string `json:"remote_addr,omitempty"`
+ ServerAddr string `json:"server_addr,omitempty"`
+ ServerPort int32 `json:"server_port,omitempty"`
Review Comment:
[nitpick] The ServerPort field type changed from *int64 to int32. This could
be a breaking change if existing code expects the pointer type or int64 range.
Consider documenting this change or using the same type for consistency.
##########
test/e2e/scaffold/assertion.go:
##########
@@ -194,8 +212,10 @@ func (s *Scaffold) RequestAssert(r *RequestAssert) bool {
r.Checks = append(r.Checks, r.Check)
}
- return EventuallyWithOffset(1, func() error {
- req := r.request(r.Method, r.Path, r.Body)
+ return EventuallyWithOffset(1, func() (err error) {
+ reporter := &ErrorReporter{}
+
+ req := r.request(r.Method, r.Path,
r.Body).WithReporter(reporter)
Review Comment:
[nitpick] The ErrorReporter is created inside the retry loop. Consider
creating it once outside the loop and resetting its error state in each
iteration to avoid unnecessary allocations.
--
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]
