Skip to content

feat: Handling related resources which references to GatewayProxy #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 17, 2025

Conversation

ronething
Copy link
Contributor

@ronething ronething commented Apr 14, 2025

Signed-off-by: ashing [email protected]

  • Handling HTTPRoute/Gateway/Ingress/Consumer related resources for direct and indirect references to gatewayProxy
  • TODO: remove default config from config.yaml
image
  • httproute update parentRefs.
image

Copy link

github-actions bot commented Apr 14, 2025

conformance test report

apiVersion: 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.

Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
@ronething ronething marked this pull request as ready for review April 15, 2025 10:17
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
@ronething ronething requested review from Copilot and AlinsRan April 15, 2025 10:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
@ronething ronething requested a review from Copilot April 16, 2025 03:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Comment on lines +328 to +341
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 {
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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 {

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Signed-off-by: ashing <[email protected]>
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building an image will use this file.

@AlinsRan
Copy link
Contributor

What is the relationship between this PR and the secret?

@ronething
Copy link
Contributor Author

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 tctx.Secrets.

	tctx.Secrets[types.NamespacedName{
					Namespace: ns,
					Name:      secretRef.Name,
				}] = secret

@ronething ronething changed the title feat: process gateway proxy ref secret feat: process gateway proxy reference Apr 16, 2025
@ronething ronething changed the title feat: process gateway proxy reference feat: Handling related resources which references to GatewayProxy Apr 16, 2025
GlobalRules: result.GlobalRules,
PluginMetadata: result.PluginMetadata,
Services: result.Services,
SSLs: result.SSL,
Consumers: result.Consumers,
},
ResourceTypes: resourceTypes,
configs: d.getConfigs(rk),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more appropriate to handle it in one place.
For example, adding deleteConfigs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will change it later.

Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
@ronething ronething requested a review from AlinsRan April 17, 2025 01:31
return err
}

d.deleteConfigs(rk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sync fails and configs are not deleted, is this expected?

@ronething ronething merged commit cb81148 into release-v2-dev Apr 17, 2025
8 checks passed
@ronething ronething deleted the chore/gateway_ingress_contoller branch April 17, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants