Copilot commented on code in PR #2647:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2647#discussion_r2508445780
##########
api/adc/types.go:
##########
@@ -779,6 +779,7 @@ type Config struct {
ServerAddrs []string
Token string
TlsVerify bool
+ BakcnedType string
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
```suggestion
BackendType string
```
##########
internal/adc/client/executor.go:
##########
@@ -81,21 +81,21 @@ func (e *DefaultADCExecutor) runADC(ctx context.Context,
mode string, config adc
return nil
}
-func (e *DefaultADCExecutor) runForSingleServerWithTimeout(ctx
context.Context, serverAddr, mode string, config adctypes.Config, args
[]string) error {
+func (e *DefaultADCExecutor) runForSingleServerWithTimeout(ctx
context.Context, serverAddr string, config adctypes.Config, args []string)
error {
ctx, cancel := context.WithTimeout(ctx, 15*time.Second)
defer cancel()
- return e.runForSingleServer(ctx, serverAddr, mode, config, args)
+ return e.runForSingleServer(ctx, serverAddr, config, args)
}
-func (e *DefaultADCExecutor) runForSingleServer(ctx context.Context,
serverAddr, mode string, config adctypes.Config, args []string) error {
+func (e *DefaultADCExecutor) runForSingleServer(ctx context.Context,
serverAddr string, config adctypes.Config, args []string) error {
cmdArgs := append([]string{}, args...)
if !config.TlsVerify {
cmdArgs = append(cmdArgs, "--tls-skip-verify")
}
cmdArgs = append(cmdArgs, "--timeout", "15s")
- env := e.prepareEnv(serverAddr, mode, config.Token)
+ env := e.prepareEnv(serverAddr, config.BakcnedType, config.Token)
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
```suggestion
env := e.prepareEnv(serverAddr, config.BackendType, config.Token)
```
##########
internal/adc/translator/gatewayproxy.go:
##########
@@ -44,47 +45,53 @@ func (t *Translator) TranslateGatewayProxyToConfig(tctx
*provider.TranslateConte
if provider.Type != v1alpha1.ProviderTypeControlPlane ||
provider.ControlPlane == nil {
return nil, nil
}
+ cp := provider.ControlPlane
- config := types.Config{
- Name: utils.NamespacedNameKind(gatewayProxy).String(),
+ cfg := types.Config{
+ Name: utils.NamespacedNameKind(gatewayProxy).String(),
+ BakcnedType: cp.Mode,
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
```suggestion
BackendType: cp.Mode,
```
##########
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 = additionalNS
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = adminKey
Review Comment:
The condition checks if `opts.AdminKey` is not empty but then assigns the
local `adminKey` variable (which is already set on line 405). This logic
appears incorrect - if the intent is to use `opts.AdminKey` when provided, the
assignment should be `o.AdminKey = opts.AdminKey`.
```suggestion
o.AdminKey = opts.AdminKey
```
##########
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 = additionalNS
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = adminKey
+ }
+ if opts.ServiceHTTPPort != 0 {
+ o.ServiceHTTPPort = 9080
+ }
+ if opts.ServiceHTTPSPort == 0 {
+ o.ServiceHTTPSPort = 9443
+ }
+ if opts.ProviderType != "" {
+ if opts.ProviderType == framework.ProviderTypeAPISIX {
+ o.ConfigProvider = "etcd"
Review Comment:
Use the constant `framework.ConfigProviderTypeEtcd` instead of the hardcoded
string \"etcd\" for consistency with the codebase (see line 213 where the
constant is used).
```suggestion
o.ConfigProvider = framework.ConfigProviderTypeEtcd
```
##########
internal/adc/client/client.go:
##########
@@ -254,8 +255,11 @@ func (c *Client) sync(ctx context.Context, task Task)
error {
if resourceType == "" {
resourceType = "all"
}
+ if config.BakcnedType == "" {
+ config.BakcnedType = c.defaultMode
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
```suggestion
if config.BackendType == "" {
config.BackendType = c.defaultMode
```
##########
internal/adc/client/executor.go:
##########
@@ -250,26 +250,26 @@ func NewHTTPADCExecutor(log logr.Logger, serverURL
string, timeout time.Duration
}
// Execute implements the ADCExecutor interface using HTTP calls
-func (e *HTTPADCExecutor) Execute(ctx context.Context, mode string, config
adctypes.Config, args []string) error {
- return e.runHTTPSync(ctx, mode, config, args)
+func (e *HTTPADCExecutor) Execute(ctx context.Context, config adctypes.Config,
args []string) error {
+ return e.runHTTPSync(ctx, config, args)
}
// runHTTPSync performs HTTP sync to ADC Server for each server address
-func (e *HTTPADCExecutor) runHTTPSync(ctx context.Context, mode string, config
adctypes.Config, args []string) error {
+func (e *HTTPADCExecutor) runHTTPSync(ctx context.Context, config
adctypes.Config, args []string) error {
var execErrs = types.ADCExecutionError{
Name: config.Name,
}
serverAddrs := func() []string {
- if mode == "apisix-standalone" {
+ if config.BakcnedType == "apisix-standalone" {
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
```suggestion
if config.BackendType == "apisix-standalone" {
```
##########
internal/adc/client/executor.go:
##########
@@ -379,13 +379,13 @@ func (e *HTTPADCExecutor) loadResourcesFromFile(filePath
string) (*adctypes.Reso
}
// buildHTTPRequest builds the HTTP request for ADC Server
-func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr,
mode string, config adctypes.Config, labels map[string]string, types []string,
resources *adctypes.Resources) (*http.Request, error) {
+func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr
string, config adctypes.Config, labels map[string]string, types []string,
resources *adctypes.Resources) (*http.Request, error) {
// Prepare request body
tlsVerify := config.TlsVerify
reqBody := ADCServerRequest{
Task: ADCServerTask{
Opts: ADCServerOpts{
- Backend: mode,
+ Backend: config.BakcnedType,
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
##########
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 = additionalNS
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = adminKey
+ }
+ if opts.ServiceHTTPPort != 0 {
+ o.ServiceHTTPPort = 9080
+ }
+ if opts.ServiceHTTPSPort == 0 {
+ o.ServiceHTTPSPort = 9443
Review Comment:
The condition logic is inverted - it checks if the port IS zero (not
provided), but the pattern used for other fields suggests this should check if
it's NOT zero. Additionally, the assignment should be `o.ServiceHTTPSPort =
opts.ServiceHTTPSPort` to use the provided value.
```suggestion
if opts.ServiceHTTPSPort != 0 {
o.ServiceHTTPSPort = opts.ServiceHTTPSPort
```
##########
internal/adc/client/executor.go:
##########
@@ -407,7 +407,7 @@ func (e *HTTPADCExecutor) buildHTTPRequest(ctx
context.Context, serverAddr, mode
e.log.V(1).Info("sending HTTP request to ADC Server",
"url", e.serverURL+"/sync",
"server", serverAddr,
- "mode", mode,
+ "mode", config.BakcnedType,
Review Comment:
Corrected spelling of 'BakcnedType' to 'BackendType'.
##########
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 = additionalNS
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = adminKey
+ }
+ if opts.ServiceHTTPPort != 0 {
+ o.ServiceHTTPPort = 9080
Review Comment:
The condition checks if `opts.ServiceHTTPPort` is not zero but then assigns
the hardcoded value 9080 (which is already set on line 406). This logic appears
incorrect - if the intent is to use `opts.ServiceHTTPPort` when provided, the
assignment should be `o.ServiceHTTPPort = opts.ServiceHTTPPort`.
```suggestion
o.ServiceHTTPPort = opts.ServiceHTTPPort
```
##########
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 = additionalNS
Review Comment:
The condition checks if `opts.Namespace` is not empty but then assigns
`additionalNS` (which is already set on line 404). This logic appears incorrect
- if the intent is to use `opts.Namespace` when provided, the assignment should
be `o.Namespace = opts.Namespace`.
```suggestion
o.Namespace = opts.Namespace
```
##########
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 = additionalNS
+ }
+ if opts.AdminKey != "" {
+ o.AdminKey = adminKey
+ }
+ if opts.ServiceHTTPPort != 0 {
+ o.ServiceHTTPPort = 9080
+ }
+ if opts.ServiceHTTPSPort == 0 {
+ o.ServiceHTTPSPort = 9443
+ }
+ if opts.ProviderType != "" {
+ if opts.ProviderType == framework.ProviderTypeAPISIX {
+ o.ConfigProvider = "etcd"
+ } else {
+ o.ConfigProvider = "yaml"
Review Comment:
Use the constant `framework.ConfigProviderTypeYaml` instead of the hardcoded
string \"yaml\" for consistency with the codebase (see line 211 where the
constant is used).
```suggestion
o.ConfigProvider = framework.ConfigProviderTypeYaml
```
--
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]
