Skip to content

Conversation

shuqz
Copy link
Collaborator

@shuqz shuqz commented Jul 21, 2025

Description

  • implement grpcroute match

  • refactor code in rule precedence file, the big changes are

    • grpcroute and httproute has independent compare precedence logic
    • the sort logic will take all routes from same port but it will sort grpcroute and httproute independently, and append them together. based on current logic, rules from httproute will be appended first since they tend to be more specific, but we might plan to have it configurable by user later. This is to handle the case when both grpcroute and httproute attached to same port, and for old logic, if will try to compare a rule from grpcroute and a rule from httproute, which does not make sense since they are different. Also, because the only time both can be attached is when hostname does not overlap, so they will never have same routing rule. we just need to keep the relative priority within each route to be correct.
  • change priority of prefix and regex for easy of comparison, before it is exact > regex > prefix, now it is exact > prefix > regex (align with ingress)

  • did not add e2e test yet, did manual testing and run some existing e2e test

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the docs directory)
  • Manually tested
    tested with 2 yaml files below
  rules:
    - matches:
        - method:
            # provide only method
            method: User
        - headers:
            # no method provided, will be treated as /*
            - name: test-header-4
              value: test-value-4
      backendRefs:
        - name: echoserver3
          port: 80
    - matches:
        - method:
            # provide both service and method
            service: com.example
            method: Login
          headers:
            - name: test-header-1
              value: test-value-1
            - name: test-header-2
              value: test-value-2
        - method:
            # provide only service
            service: second.com
          headers:
            - name: test-header-3
              value: test-value-3
-----------------------------------
  rules:
    - matches:
        - method:
            # provide only method
            type: RegularExpression
            method: User*
          headers:
            - name: test-header-5
              value: test-value-5
        - method:
            # provide only service
            type: RegularExpression
            service: service?
          headers:
            - name: test-header-6
              value: test-value-6

result:
Screenshot 2025-07-24 at 11 04 26 AM

  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot k8s-ci-robot requested a review from M00nF1sh July 21, 2025 16:15
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2025
@k8s-ci-robot k8s-ci-robot requested a review from zac-nixon July 21, 2025 16:15
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 21, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2025
[feat-api] refactor rule precedence code
@shuqz shuqz force-pushed the shuqz-grpcroute branch from a09a974 to 1ab82f7 Compare July 24, 2025 17:57
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 24, 2025
@shraddhabang
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2025
@shraddhabang shraddhabang added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: shuqz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@shuqz
Copy link
Collaborator Author

shuqz commented Jul 24, 2025

/retest

1 similar comment
@shraddhabang
Copy link
Collaborator

/retest

@shraddhabang
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2025
@k8s-ci-robot k8s-ci-robot merged commit 7963004 into kubernetes-sigs:main Jul 25, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants