Copilot commented on code in PR #2600:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2600#discussion_r2426066583
##########
test/e2e/webhook/gatewayproxy.go:
##########
@@ -121,9 +122,151 @@ stringData:
err = s.DeleteResource("GatewayProxy", gpName)
Expect(err).ShouldNot(HaveOccurred())
- output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gpWithSecrets, gpName,
missingService, missingSecret))
+ output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gatewayProxyTemplate,
gpName, missingService, missingSecret))
Expect(err).ShouldNot(HaveOccurred())
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Referenced Service '%s/%s' not found", s.Namespace(), missingService)))
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Secret key 'token' not found in Secret '%s/%s'", s.Namespace(), missingSecret)))
})
+
+ Context("GatewayProxy configuration conflicts", func() {
+ It("should reject GatewayProxy that reuses the same Service and
AdminKey Secret as an existing one on create and update", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ serviceName := "gatewayproxy-shared-service"
+ secretName := "gatewayproxy-shared-secret"
+ initialProxy := "gatewayproxy-shared-primary"
+ conflictingProxy := "gatewayproxy-shared-conflict"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
serviceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, secretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, initialProxy,
serviceName, secretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating initial
GatewayProxy")
+
+ time.Sleep(2 * time.Second)
Review Comment:
Hard-coded sleep statements in tests are fragile and slow down test
execution. Consider using a polling mechanism or event-driven approach to wait
for the resource to be properly registered in the webhook system.
```suggestion
Eventually(func() error {
// Replace with a check for GatewayProxy
readiness/existence.
// If s.GetResource returns error when not
found, this works.
return s.GetResource("GatewayProxy",
initialProxy)
}, 10*time.Second,
500*time.Millisecond).Should(Succeed(), "waiting for initial GatewayProxy to be
registered")
```
##########
test/e2e/webhook/gatewayproxy.go:
##########
@@ -121,9 +122,151 @@ stringData:
err = s.DeleteResource("GatewayProxy", gpName)
Expect(err).ShouldNot(HaveOccurred())
- output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gpWithSecrets, gpName,
missingService, missingSecret))
+ output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gatewayProxyTemplate,
gpName, missingService, missingSecret))
Expect(err).ShouldNot(HaveOccurred())
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Referenced Service '%s/%s' not found", s.Namespace(), missingService)))
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Secret key 'token' not found in Secret '%s/%s'", s.Namespace(), missingSecret)))
})
+
+ Context("GatewayProxy configuration conflicts", func() {
+ It("should reject GatewayProxy that reuses the same Service and
AdminKey Secret as an existing one on create and update", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ serviceName := "gatewayproxy-shared-service"
+ secretName := "gatewayproxy-shared-secret"
+ initialProxy := "gatewayproxy-shared-primary"
+ conflictingProxy := "gatewayproxy-shared-conflict"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
serviceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, secretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, initialProxy,
serviceName, secretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating initial
GatewayProxy")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, conflictingProxy,
serviceName, secretName))
+ Expect(err).Should(HaveOccurred(), "expecting conflict
for duplicated GatewayProxy")
+ Expect(err.Error()).To(ContainSubstring("gateway proxy
configuration conflict"))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
conflictingProxy)))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
initialProxy)))
+ Expect(err.Error()).To(ContainSubstring("Service"))
+ Expect(err.Error()).To(ContainSubstring("AdminKey
secret"))
+
+ Expect(s.DeleteResource("GatewayProxy",
initialProxy)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Service",
serviceName)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Secret",
secretName)).ShouldNot(HaveOccurred())
+ })
+
+ It("should reject GatewayProxy that overlaps endpoints when
sharing inline AdminKey value", func() {
+ gatewayProxyTemplate := `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: %s
+spec:
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+
+ existingProxy := "gatewayproxy-inline-primary"
+ conflictingProxy := "gatewayproxy-inline-conflict"
+ endpointA := "https://127.0.0.1:9443";
+ endpointB := "https://10.0.0.1:9443";
+ endpointC := "https://192.168.0.1:9443";
+ inlineKey := "inline-credential"
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, existingProxy,
endpointA, endpointB, inlineKey))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy with inline AdminKey")
+
+ time.Sleep(2 * time.Second)
Review Comment:
Multiple hard-coded sleep statements suggest a systematic issue with test
timing. Consider implementing a helper function that polls for resource
readiness or uses the Kubernetes client's watch functionality to wait for
proper resource state.
##########
test/e2e/webhook/gatewayproxy.go:
##########
@@ -121,9 +122,151 @@ stringData:
err = s.DeleteResource("GatewayProxy", gpName)
Expect(err).ShouldNot(HaveOccurred())
- output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gpWithSecrets, gpName,
missingService, missingSecret))
+ output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gatewayProxyTemplate,
gpName, missingService, missingSecret))
Expect(err).ShouldNot(HaveOccurred())
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Referenced Service '%s/%s' not found", s.Namespace(), missingService)))
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Secret key 'token' not found in Secret '%s/%s'", s.Namespace(), missingSecret)))
})
+
+ Context("GatewayProxy configuration conflicts", func() {
+ It("should reject GatewayProxy that reuses the same Service and
AdminKey Secret as an existing one on create and update", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ serviceName := "gatewayproxy-shared-service"
+ secretName := "gatewayproxy-shared-secret"
+ initialProxy := "gatewayproxy-shared-primary"
+ conflictingProxy := "gatewayproxy-shared-conflict"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
serviceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, secretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, initialProxy,
serviceName, secretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating initial
GatewayProxy")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, conflictingProxy,
serviceName, secretName))
+ Expect(err).Should(HaveOccurred(), "expecting conflict
for duplicated GatewayProxy")
+ Expect(err.Error()).To(ContainSubstring("gateway proxy
configuration conflict"))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
conflictingProxy)))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
initialProxy)))
+ Expect(err.Error()).To(ContainSubstring("Service"))
+ Expect(err.Error()).To(ContainSubstring("AdminKey
secret"))
+
+ Expect(s.DeleteResource("GatewayProxy",
initialProxy)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Service",
serviceName)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Secret",
secretName)).ShouldNot(HaveOccurred())
+ })
+
+ It("should reject GatewayProxy that overlaps endpoints when
sharing inline AdminKey value", func() {
+ gatewayProxyTemplate := `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: %s
+spec:
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+
+ existingProxy := "gatewayproxy-inline-primary"
+ conflictingProxy := "gatewayproxy-inline-conflict"
+ endpointA := "https://127.0.0.1:9443";
+ endpointB := "https://10.0.0.1:9443";
+ endpointC := "https://192.168.0.1:9443";
+ inlineKey := "inline-credential"
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, existingProxy,
endpointA, endpointB, inlineKey))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy with inline AdminKey")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, conflictingProxy,
endpointB, endpointC, inlineKey))
+ Expect(err).Should(HaveOccurred(), "expecting conflict
for overlapping endpoints with shared AdminKey")
+ Expect(err.Error()).To(ContainSubstring("gateway proxy
configuration conflict"))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
conflictingProxy)))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
existingProxy)))
+ Expect(err.Error()).To(ContainSubstring("control plane
endpoints"))
+ Expect(err.Error()).To(ContainSubstring("inline
AdminKey value"))
+ })
+
+ It("should reject GatewayProxy update that creates conflict
with another GatewayProxy", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ sharedServiceName :=
"gatewayproxy-update-shared-service"
+ sharedSecretName := "gatewayproxy-update-shared-secret"
+ uniqueServiceName :=
"gatewayproxy-update-unique-service"
+ proxyA := "gatewayproxy-update-a"
+ proxyB := "gatewayproxy-update-b"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
sharedServiceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
uniqueServiceName))).ShouldNot(HaveOccurred(), "creating unique Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, sharedSecretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, proxyA,
sharedServiceName, sharedSecretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy A with shared Service and Secret")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, proxyB,
uniqueServiceName, sharedSecretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy B with unique Service but same Secret")
+
+ time.Sleep(2 * time.Second)
Review Comment:
Multiple hard-coded sleep statements suggest a systematic issue with test
timing. Consider implementing a helper function that polls for resource
readiness or uses the Kubernetes client's watch functionality to wait for
proper resource state.
##########
test/e2e/webhook/gatewayproxy.go:
##########
@@ -121,9 +122,151 @@ stringData:
err = s.DeleteResource("GatewayProxy", gpName)
Expect(err).ShouldNot(HaveOccurred())
- output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gpWithSecrets, gpName,
missingService, missingSecret))
+ output, err =
s.CreateResourceFromStringAndGetOutput(fmt.Sprintf(gatewayProxyTemplate,
gpName, missingService, missingSecret))
Expect(err).ShouldNot(HaveOccurred())
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Referenced Service '%s/%s' not found", s.Namespace(), missingService)))
Expect(output).NotTo(ContainSubstring(fmt.Sprintf("Warning:
Secret key 'token' not found in Secret '%s/%s'", s.Namespace(), missingSecret)))
})
+
+ Context("GatewayProxy configuration conflicts", func() {
+ It("should reject GatewayProxy that reuses the same Service and
AdminKey Secret as an existing one on create and update", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ serviceName := "gatewayproxy-shared-service"
+ secretName := "gatewayproxy-shared-secret"
+ initialProxy := "gatewayproxy-shared-primary"
+ conflictingProxy := "gatewayproxy-shared-conflict"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
serviceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, secretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, initialProxy,
serviceName, secretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating initial
GatewayProxy")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, conflictingProxy,
serviceName, secretName))
+ Expect(err).Should(HaveOccurred(), "expecting conflict
for duplicated GatewayProxy")
+ Expect(err.Error()).To(ContainSubstring("gateway proxy
configuration conflict"))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
conflictingProxy)))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
initialProxy)))
+ Expect(err.Error()).To(ContainSubstring("Service"))
+ Expect(err.Error()).To(ContainSubstring("AdminKey
secret"))
+
+ Expect(s.DeleteResource("GatewayProxy",
initialProxy)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Service",
serviceName)).ShouldNot(HaveOccurred())
+ Expect(s.DeleteResource("Secret",
secretName)).ShouldNot(HaveOccurred())
+ })
+
+ It("should reject GatewayProxy that overlaps endpoints when
sharing inline AdminKey value", func() {
+ gatewayProxyTemplate := `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: %s
+spec:
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+
+ existingProxy := "gatewayproxy-inline-primary"
+ conflictingProxy := "gatewayproxy-inline-conflict"
+ endpointA := "https://127.0.0.1:9443";
+ endpointB := "https://10.0.0.1:9443";
+ endpointC := "https://192.168.0.1:9443";
+ inlineKey := "inline-credential"
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, existingProxy,
endpointA, endpointB, inlineKey))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy with inline AdminKey")
+
+ time.Sleep(2 * time.Second)
+
+ err =
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, conflictingProxy,
endpointB, endpointC, inlineKey))
+ Expect(err).Should(HaveOccurred(), "expecting conflict
for overlapping endpoints with shared AdminKey")
+ Expect(err.Error()).To(ContainSubstring("gateway proxy
configuration conflict"))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
conflictingProxy)))
+
Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("%s/%s", s.Namespace(),
existingProxy)))
+ Expect(err.Error()).To(ContainSubstring("control plane
endpoints"))
+ Expect(err.Error()).To(ContainSubstring("inline
AdminKey value"))
+ })
+
+ It("should reject GatewayProxy update that creates conflict
with another GatewayProxy", func() {
+ serviceTemplate := `
+apiVersion: v1
+kind: Service
+metadata:
+ name: %s
+spec:
+ selector:
+ app: dummy-control-plane
+ ports:
+ - name: admin
+ port: 9180
+ targetPort: 9180
+`
+ secretTemplate := `
+apiVersion: v1
+kind: Secret
+metadata:
+ name: %s
+type: Opaque
+stringData:
+ %s: %s
+`
+ sharedServiceName :=
"gatewayproxy-update-shared-service"
+ sharedSecretName := "gatewayproxy-update-shared-secret"
+ uniqueServiceName :=
"gatewayproxy-update-unique-service"
+ proxyA := "gatewayproxy-update-a"
+ proxyB := "gatewayproxy-update-b"
+
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
sharedServiceName))).ShouldNot(HaveOccurred(), "creating shared Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(serviceTemplate,
uniqueServiceName))).ShouldNot(HaveOccurred(), "creating unique Service")
+
Expect(s.CreateResourceFromString(fmt.Sprintf(secretTemplate, sharedSecretName,
"token", "value"))).ShouldNot(HaveOccurred(), "creating shared Secret")
+
+ err :=
s.CreateResourceFromString(fmt.Sprintf(gatewayProxyTemplate, proxyA,
sharedServiceName, sharedSecretName))
+ Expect(err).ShouldNot(HaveOccurred(), "creating
GatewayProxy A with shared Service and Secret")
+
+ time.Sleep(2 * time.Second)
Review Comment:
Multiple hard-coded sleep statements suggest a systematic issue with test
timing. Consider implementing a helper function that polls for resource
readiness or uses the Kubernetes client's watch functionality to wait for
proper resource state.
--
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]
