feat: Handling related resources which references to GatewayProxy#90
Conversation
Signed-off-by: ashing <axingfly@gmail.com>
…ay_ingress_contoller
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2025-04-16T16:18:16Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
contact: null
organization: API7
project: api7-ingress-controller
url: https://github.com/api7/api7-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- HTTPRoutePathMatchOrder
- HTTPRouteServiceTypes
- HTTPRouteSimpleSameNamespace
result: failure
skippedTests:
- GatewayInvalidTLSConfiguration
- GatewaySecretInvalidReferenceGrant
- GatewaySecretMissingReferenceGrant
- GatewaySecretReferenceGrantAllInNamespace
- GatewaySecretReferenceGrantSpecific
- HTTPRouteExactPathMatching
- HTTPRouteHTTPSListener
- HTTPRouteHeaderMatching
- HTTPRouteHostnameIntersection
- HTTPRouteInvalidBackendRefUnknownKind
- HTTPRouteInvalidCrossNamespaceBackendRef
- HTTPRouteInvalidCrossNamespaceParentRef
- HTTPRouteInvalidNonExistentBackendRef
- HTTPRouteInvalidParentRefNotMatchingSectionName
- HTTPRouteInvalidReferenceGrant
- HTTPRouteListenerHostnameMatching
- HTTPRouteMatching
- HTTPRouteMatchingAcrossRoutes
- HTTPRoutePartiallyInvalidViaInvalidReferenceGrant
- HTTPRouteReferenceGrant
- HTTPRouteRequestHeaderModifier
- HTTPRouteWeight
statistics:
Failed: 3
Passed: 8
Skipped: 22
name: GATEWAY-HTTP
summary: Core tests failed with 3 test failures. |
There was a problem hiding this comment.
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
internal/provider/adc/translator/gateway.go:37
- [nitpick] Consider explicitly handling the scenario when multiple GatewayProxy resources are present instead of arbitrarily selecting the first element. Clarifying this behavior (or adding support for multiple proxies if intended) would improve the maintainability and readability of the translation logic.
gatewayProxy = &tctx.GatewayProxies[0]
There was a problem hiding this comment.
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/provider/adc/translator/gateway.go:36
- [nitpick] The logic here selects the first GatewayProxy from the GatewayProxies slice; if multiple proxies are provided, please ensure this behavior is intentional or add documentation to clarify the selection criteria.
if len(tctx.GatewayProxies) > 0 {
| if len(task.configs) > 0 { | ||
| log.Debugw("syncing resources with multiple configs", zap.Any("configs", task.configs)) | ||
| for _, config := range task.configs { | ||
| if err := d.execADC(config, args); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } else { | ||
| // todo: remove using default config | ||
| log.Debugw("syncing resources with default config") | ||
| if err := d.execADC(adcConfig{ | ||
| ServerAddr: d.ServerAddr, | ||
| Token: d.Token, | ||
| }, args); err != nil { |
There was a problem hiding this comment.
[nitpick] Falling back to a default ADC configuration when task.configs is empty could lead to unintended behavior; consider enforcing explicit configuration or better handling of the empty case.
| if len(task.configs) > 0 { | |
| log.Debugw("syncing resources with multiple configs", zap.Any("configs", task.configs)) | |
| for _, config := range task.configs { | |
| if err := d.execADC(config, args); err != nil { | |
| return err | |
| } | |
| } | |
| } else { | |
| // todo: remove using default config | |
| log.Debugw("syncing resources with default config") | |
| if err := d.execADC(adcConfig{ | |
| ServerAddr: d.ServerAddr, | |
| Token: d.Token, | |
| }, args); err != nil { | |
| if len(task.configs) == 0 { | |
| log.Errorw("task.configs is empty; explicit configuration is required") | |
| return errors.New("task.configs cannot be empty; explicit configuration is required") | |
| } | |
| log.Debugw("syncing resources with multiple configs", zap.Any("configs", task.configs)) | |
| for _, config := range task.configs { | |
| if err := d.execADC(config, args); err != nil { |
| @@ -1,4 +1,4 @@ | |||
| log_level: "info" # The log level of the API7 Ingress Controller. | |||
| log_level: "debug" # The log level of the API7 Ingress Controller. | |||
There was a problem hiding this comment.
Yes, it can also be considered to be changed back, but most of the time we are using it for debugging?Will users directly use this file?
There was a problem hiding this comment.
Building an image will use this file.
|
What is the relationship between this PR and the secret? |
maybe we can change a PR title. Initially, it was for handling various reference relationships, which included the GatewayProxy reference secret that needed to be recorded in |
| Consumers: result.Consumers, | ||
| }, | ||
| ResourceTypes: resourceTypes, | ||
| configs: d.getConfigs(rk), |
There was a problem hiding this comment.
I think it would be more appropriate to handle it in one place.
For example, adding deleteConfigs
There was a problem hiding this comment.
ok, i will change it later.
Signed-off-by: ashing <axingfly@gmail.com>
…ay_ingress_contoller Signed-off-by: ashing <axingfly@gmail.com>
| return err | ||
| } | ||
|
|
||
| d.deleteConfigs(rk) |
There was a problem hiding this comment.
If sync fails and configs are not deleted, is this expected?
Signed-off-by: ashing axingfly@gmail.com