Skip to content

Conversation

@ronething
Copy link
Contributor

@ronething ronething commented May 6, 2025

{
    Request:   http.Request{Path: "/", Headers: map[string]string{"Version": "two"}},
    Backend:   "infra-backend-v2",
    Namespace: ns,
},
                
{
    // Not a path segment prefix so should not match /v2.
    Request:   http.Request{Path: "/v2example"},
    Backend:   "infra-backend-v1",
    Namespace: ns,
}

Handle the above two cases

ref: https://gateway-api.sigs.k8s.io/reference/spec/#httprouterule

@ronething ronething requested a review from Copilot May 6, 2025 03:23
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 addresses issues with HTTPRoute matching by modifying the matching logic and adjusting test coverage accordingly.

  • Disables a failing or redundant HTTPRouteMatching test in conformance tests.
  • Updates URI matching logic in the translator to better handle trailing slashes and wildcard appending.
  • Refactors header and query parameter matching to safely handle nil pointers.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/conformance/conformance_test.go Disables the HTTPRouteMatching test to reflect new logic
internal/provider/adc/translator/httproute.go Updates path prefix matching behavior and improves header/query matching handling
Comments suppressed due to low confidence (1)

test/conformance/conformance_test.go:40

  • Disabling the HTTPRouteMatching test may reduce test coverage. Please confirm that this test is redundant or covered by other tests and consider adding an inline comment explaining the reason for disabling.
// tests.HTTPRouteMatching.ShortName,

route.Uris = []string{*match.Path.Value + "*"}
pathValue := *match.Path.Value
route.Uris = []string{pathValue}

Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

The new logic separates the original path and its wildcard extension into two URIs. Please add inline documentation to clarify the intent and ensure that this change aligns with the expected backend matching behavior.

Suggested change
// Append wildcard extensions to ensure the route matches both the base path
// and any subpaths. For example, a path "/example" will match "/example"
// and "/example/*". This behavior aligns with the backend's expectation
// for prefix-based path matching.

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

github-actions bot commented May 6, 2025

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2025-05-06T04:23:51Z"
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:
    result: partial
    skippedTests:
    - GatewayInvalidTLSConfiguration
    - GatewaySecretInvalidReferenceGrant
    - GatewaySecretMissingReferenceGrant
    - HTTPRouteHTTPSListener
    - HTTPRouteHostnameIntersection
    - HTTPRouteInvalidBackendRefUnknownKind
    - HTTPRouteInvalidCrossNamespaceBackendRef
    - HTTPRouteInvalidCrossNamespaceParentRef
    - HTTPRouteInvalidNonExistentBackendRef
    - HTTPRouteInvalidParentRefNotMatchingSectionName
    - HTTPRouteInvalidReferenceGrant
    - HTTPRouteListenerHostnameMatching
    - HTTPRoutePartiallyInvalidViaInvalidReferenceGrant
    - HTTPRouteReferenceGrant
    statistics:
      Failed: 0
      Passed: 19
      Skipped: 14
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 14 test skips.

Signed-off-by: ashing <[email protected]>
@ronething ronething requested a review from Copilot May 6, 2025 04:25
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 pull request fixes httproute matching, addressing issues where HTTP routes were incorrectly prioritized and URIs not properly constructed for prefix matches. Key changes include disabling outdated conformance tests, adding match priority calculation and sorting logic, and refining URI construction based on path formatting.

Reviewed Changes

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

File Description
test/conformance/conformance_test.go Commented out tests related to HTTPRouteMatching to bypass failing behavior temporarily
internal/provider/adc/translator/httproute.go Introduced match priority calculation, improved URI construction, and refined header/query match handling
Comments suppressed due to low confidence (1)

test/conformance/conformance_test.go:40

  • Commenting out these conformance tests reduces test coverage for HTTPRouteMatching behavior; please consider adding a TODO or reference to an issue to ensure they are revisited once the new matching logic is validated.
tests.HTTPRouteMatching.ShortName,

route.Uris = []string{*match.Path.Value + "*"}
pathValue := *match.Path.Value
route.Uris = []string{pathValue}

Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The logic for constructing route URIs based on whether the path ends with a slash is not immediately obvious; consider adding an inline comment to clarify why a different wildcard is appended depending on the trailing slash.

Suggested change
// If the path ends with a slash, append "*" to match all subpaths.
// Otherwise, append "/*" to match the path itself and all its subpaths.

Copilot uses AI. Check for mistakes.
this = append(this, adctypes.StringOrSlice{
StrVal: "http_" + name,
})

Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The default assignment of the header match type improves clarity, but adding a brief comment explaining this default value could help future maintainers understand the decision process.

Suggested change
// Default to HeaderMatchExact if no specific header match type is provided.

Copilot uses AI. Check for mistakes.
@ronething ronething requested a review from AlinsRan May 6, 2025 10:29
@ronething ronething merged commit 4248014 into release-v2-dev May 6, 2025
8 checks passed
@ronething ronething deleted the fix/httproutematching branch May 6, 2025 10:41
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