Skip to content

Conversation

@ronething
Copy link
Contributor

Signed-off-by: ashing [email protected]

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

ronething added 3 commits May 9, 2025 10:48
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
@ronething ronething requested review from AlinsRan and Copilot May 9, 2025 03:46
Copy link
Contributor

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.

Pull Request Overview

This PR fixes the backend reference test case by updating the test suite as well as modifying error handling in backend reference translation and HTTPRoute reconciliation.

  • Commented out the backendRef unknown kind test in the conformance tests.
  • Updated translateBackendRef to return errors when an unsupported backend kind is provided and enhanced error handling in the HTTPRoute controller.
  • Revised the license header in generated deepcopy files.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/conformance/conformance_test.go Commented out the problematic backend reference test case to reflect updated behavior.
internal/provider/adc/translator/httproute.go Modified translateBackendRef to validate backend kinds and return detailed errors.
internal/controller/utils.go Adjusted condition reason assignment based on the error message content.
internal/controller/httproute_controller.go Enhanced HTTPRoute reconciliation logic to propagate backend reference errors appropriately.
api/dashboard/v1/zz_generated.deepcopy.go Updated the license header formatting.


tests.GatewayInvalidTLSConfiguration.ShortName,
tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName,
// tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName,
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding an inline comment explaining why the HTTPRouteInvalidBackendRefUnknownKind test is commented out to clarify that this change is intentional.

Suggested change
// tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName,
// tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName, // Commented out because this test is not applicable to the current implementation. See issue tracker for details.

Copilot uses AI. Check for mistakes.
func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) adctypes.UpstreamNodes {
func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) {
if ref.Kind != nil && *ref.Kind != "Service" {
return adctypes.UpstreamNodes{}, fmt.Errorf("kind %s is not supported", *ref.Kind)
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

Enhance the error message here by including additional context (e.g., backend reference name or namespace) to improve debugging when an unsupported kind is encountered.

Suggested change
return adctypes.UpstreamNodes{}, fmt.Errorf("kind %s is not supported", *ref.Kind)
key := types.NamespacedName{
Namespace: string(*ref.Namespace),
Name: string(ref.Name),
}
return adctypes.UpstreamNodes{}, fmt.Errorf("kind %s is not supported for backend reference %s", *ref.Kind, key)

Copilot uses AI. Check for mistakes.
}

// If the backend reference error is because of an invalid kind, use this error first
if httpRouteErr != nil && strings.Contains(httpRouteErr.Error(), string(gatewayv1.RouteReasonInvalidKind)) {
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding logging for the backend reference error (httpRouteErr) before setting the resolveRefStatus so that the issue is more easily traceable in production environments.

Suggested change
if httpRouteErr != nil && strings.Contains(httpRouteErr.Error(), string(gatewayv1.RouteReasonInvalidKind)) {
if httpRouteErr != nil && strings.Contains(httpRouteErr.Error(), string(gatewayv1.RouteReasonInvalidKind)) {
r.Log.Error(httpRouteErr, "Backend reference error due to invalid kind encountered while processing HTTPRoute")

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2025

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2025-05-09T06:57:58Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: partial
    skippedTests:
    - GatewayInvalidTLSConfiguration
    - GatewaySecretInvalidReferenceGrant
    - GatewaySecretMissingReferenceGrant
    - HTTPRouteHTTPSListener
    - HTTPRouteHostnameIntersection
    - HTTPRouteInvalidCrossNamespaceBackendRef
    - HTTPRouteInvalidCrossNamespaceParentRef
    - HTTPRouteInvalidNonExistentBackendRef
    - HTTPRouteInvalidParentRefNotMatchingSectionName
    - HTTPRouteInvalidReferenceGrant
    - HTTPRouteListenerHostnameMatching
    - HTTPRoutePartiallyInvalidViaInvalidReferenceGrant
    - HTTPRouteReferenceGrant
    statistics:
      Failed: 0
      Passed: 20
      Skipped: 13
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 13 test skips.

@ronething ronething merged commit 80d84b9 into release-v2-dev May 9, 2025
8 of 9 checks passed
@ronething ronething deleted the fix/hostname branch May 9, 2025 08:24
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