Copilot commented on code in PR #2647:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2647#discussion_r2508467060
##########
config/crd/bases/apisix.apache.org_gatewayproxies.yaml:
##########
@@ -127,6 +127,11 @@ spec:
type: string
minItems: 1
type: array
+ mode:
+ description: |-
+ Mode specifies the mode of control plane provider.
+ Can be `apisix` or `apisix-standalone`.
+ type: string
Review Comment:
The `mode` field lacks enum validation in the CRD. According to the
documentation and comments, it should only accept `apisix` or
`apisix-standalone` values. Consider adding an enum constraint to prevent
invalid values:
```yaml
mode:
description: |-
Mode specifies the mode of control plane provider.
Can be `apisix` or `apisix-standalone`.
type: string
enum:
- apisix
- apisix-standalone
```
```suggestion
type: string
enum:
- apisix
- apisix-standalone
```
##########
api/v1alpha1/gatewayproxy_types.go:
##########
@@ -119,7 +119,13 @@ type ControlPlaneAuth struct {
// ControlPlaneProvider defines configuration for control plane provider.
// +kubebuilder:validation:XValidation:rule="has(self.endpoints) !=
has(self.service)"
+// +kubebuilder:validation:XValidation:rule="oldSelf == null ||
(!has(self.mode) && !has(oldSelf.mode)) || self.mode ==
oldSelf.mode",message="mode is immutable"
type ControlPlaneProvider struct {
+ // Mode specifies the mode of control plane provider.
+ // Can be `apisix` or `apisix-standalone`.
+ //
+ // +kubebuilder:validation:Optional
Review Comment:
The `Mode` field in the Go type should include a kubebuilder validation enum
to match the documented valid values. Consider adding:
```go
// Mode specifies the mode of control plane provider.
// Can be `apisix` or `apisix-standalone`.
//
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Enum=apisix;apisix-standalone
Mode string `json:"mode,omitempty"`
```
```suggestion
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Enum=apisix;apisix-standalone
```
##########
test/e2e/scaffold/apisix_deployer.go:
##########
@@ -371,6 +375,77 @@ func (s *APISIXDeployer)
CreateAdditionalGateway(namePrefix string) (string, *co
return identifier, svc, nil
}
+func (s *APISIXDeployer) CreateAdditionalGatewayWithOptions(namePrefix string,
opts DeployDataplaneOptions) (string, *corev1.Service, error) {
+ // Create a new namespace for this additional gateway
+ additionalNS := fmt.Sprintf("%s-%d", namePrefix, time.Now().Unix())
+
+ k8s.CreateNamespace(s.t, s.kubectlOptions, additionalNS)
+
+ // Create new kubectl options for the new namespace
+ kubectlOpts := &k8s.KubectlOptions{
+ ConfigPath: s.runtimeOpts.Kubeconfig,
+ Namespace: additionalNS,
+ }
+
+ s.Logf("additional gateway in namespace %s", additionalNS)
+
+ // Use the same admin key as the main gateway
+ adminKey := s.runtimeOpts.APISIXAdminAPIKey
+ s.Logf("additional gateway admin api key: %s", adminKey)
+
+ // Store gateway resources info
+ resources := &GatewayResources{
+ Namespace: additionalNS,
+ AdminAPIKey: adminKey,
+ }
+
+ // Deploy dataplane for this additional gateway
+ o := APISIXDeployOptions{
+ Namespace: additionalNS,
+ AdminKey: adminKey,
+ ServiceHTTPPort: 9080,
+ ServiceHTTPSPort: 9443,
+ }
+ if opts.Namespace != "" {
+ o.Namespace = opts.Namespace
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = opts.AdminKey
+ }
+ if opts.ServiceHTTPPort != 0 {
+ o.ServiceHTTPPort = opts.ServiceHTTPPort
+ }
+ if opts.ServiceHTTPSPort != 0 {
+ o.ServiceHTTPSPort = opts.ServiceHTTPSPort
+ }
+ if opts.ProviderType != "" {
+ if opts.ProviderType == framework.ProviderTypeAPISIX {
+ o.ConfigProvider = framework.ConfigProviderTypeEtcd
+ } else {
+ o.ConfigProvider = framework.ConfigProviderTypeYaml
+ }
+ }
+ svc := s.deployDataplane(&o)
+
+ resources.DataplaneService = svc
+
+ // Create tunnels for the dataplane
+ tunnels, err := s.createDataplaneTunnels(svc, kubectlOpts, svc.Name)
+ if err != nil {
+ return "", nil, err
+ }
+
+ resources.Tunnels = tunnels
+
+ // Use namespace as identifier for APISIX deployments
+ identifier := additionalNS
+
+ // Store in the map
+ s.additionalGateways[identifier] = resources
+
+ return identifier, svc, nil
+}
Review Comment:
[nitpick] The `CreateAdditionalGatewayWithOptions` function contains
significant code duplication with `CreateAdditionalGateway` (lines 326-376).
Consider refactoring `CreateAdditionalGateway` to call
`CreateAdditionalGatewayWithOptions` with empty options:
```go
func (s *APISIXDeployer) CreateAdditionalGateway(namePrefix string) (string,
*corev1.Service, error) {
return s.CreateAdditionalGatewayWithOptions(namePrefix,
DeployDataplaneOptions{})
}
```
This would eliminate ~50 lines of duplicated code and make maintenance
easier.
--
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]
