Skip to content

[feat gw-api]handle resolvedRef for route status update and update ho… #4210

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 1 commit into from
May 30, 2025

Conversation

shuqz
Copy link
Contributor

@shuqz shuqz commented May 30, 2025

Description

  1. handle several unhandled reasons for ResolveRef false case in route status update
    • "RefNotPermitted"
    • "InvalidKind"
    • "BackendNotFound"
    • "UnsupportedProtocol" -> this will be comparing protocol version with appProtocol. during testing, figured that we are validation protocol version value to be supported values: "http1", "http2", "grpc" which should be capitalized, so fixed it in CRD
  2. modified logic for hostname precedence order
  • in function sortRoutesByHostnamePrecedence, with the original logic, it only compares first element in each list to sort them. but it should be find the highest precedence in each list first and then use highest precedence hostname to sort.
  • Also updated precedence order logic to count dot number, more dot means higher precedence, even though it might have shorter length
  1. added more unit tests for route_reconciler.go, found that a lot are uncovered before

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 - some of the status update test result
------
status:
  addresses:
  - type: Hostname
    value: internal-customized-alb-lb-name-1399439441.us-west-2.elb.amazonaws.com
  conditions:
  - lastTransitionTime: "2025-05-30T20:00:19Z"
    message: Backend Ref must be of kind 'Service'. Invalid data can be found in route
      (HTTPRoute, gateway-alb:http-app-1)
    observedGeneration: 16
    reason: ListenersNotValid
    status: "False"
    type: Accepted
  - lastTransitionTime: "2025-05-29T22:38:20Z"
    message: arn:aws:elasticloadbalancing:us-west-2:850920876970:loadbalancer/app/customized-alb-lb-name/7e7271c93476a8ed
    observedGeneration: 16
    reason: Programmed
    status: "True"
    type: Programmed
------
status:
  parents:
  - conditions:
    - lastTransitionTime: "2025-05-30T20:00:19Z"
      message: Backend Ref must be of kind 'Service' - test with route message override
      observedGeneration: 138
      reason: InvalidKind
      status: "False"
      type: Accepted
    - lastTransitionTime: "2025-05-30T20:00:19Z"
      message: Backend Ref must be of kind 'Service' - test with route message override
      observedGeneration: 138
      reason: InvalidKind
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.k8s.aws/alb
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: test-gw-alb
      port: 80
----------------
status:
  parents:
  - conditions:
    - lastTransitionTime: "2025-05-30T19:55:42Z"
      message: Service port appProtocol http is not compatible with target group protocolVersion
        GRPC
      observedGeneration: 137
      reason: UnsupportedProtocol
      status: "False"
      type: Accepted
    - lastTransitionTime: "2025-05-30T19:55:42Z"
      message: Service port appProtocol http is not compatible with target group protocolVersion
        GRPC
      observedGeneration: 137
      reason: UnsupportedProtocol
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.k8s.aws/alb
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: test-gw-alb
      port: 80
----------------
status:
  parents:
  - conditions:
    - lastTransitionTime: "2025-05-30T19:48:47Z"
      message: Service port appProtocol grpc is not compatible with target group protocolVersion
        HTTP1
      observedGeneration: 134
      reason: UnsupportedProtocol
      status: "False"
      type: Accepted
    - lastTransitionTime: "2025-05-30T19:48:47Z"
      message: Service port appProtocol grpc is not compatible with target group protocolVersion
        HTTP1
      observedGeneration: 134
      reason: UnsupportedProtocol
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.k8s.aws/alb
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: test-gw-alb
      port: 80
status:
  parents:
  - conditions:
    - lastTransitionTime: "2025-05-30T19:52:28Z"
      message: Backend Ref must be of kind 'Service'
      observedGeneration: 135
      reason: InvalidKind
      status: "False"
      type: Accepted
    - lastTransitionTime: "2025-05-30T19:52:28Z"
      message: Backend Ref must be of kind 'Service'
      observedGeneration: 135
      reason: InvalidKind
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.k8s.aws/alb
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: test-gw-alb
      port: 80

  • 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 🌟
  • refactored LoaderError, now by default it will return initial error message but if there is override by gateway message or route message (e.g. with wrapped detailed info) it will return the overridden message

@k8s-ci-robot k8s-ci-robot requested a review from oliviassss May 30, 2025 02:17
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 30, 2025
@k8s-ci-robot k8s-ci-robot requested a review from wweiwei-li May 30, 2025 02:17
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @shuqz. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 30, 2025
@zac-nixon
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 30, 2025
…stname precedence logic

fix make crds error

refactor LoaderError
Copy link
Collaborator

@zac-nixon zac-nixon left a comment

Choose a reason for hiding this comment

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

/approved
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shuqz, zac-nixon

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2025
@k8s-ci-robot k8s-ci-robot merged commit 4c5857e into kubernetes-sigs:main May 30, 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants