Skip to content

Conversation

zac-nixon
Copy link
Collaborator

Description

When no matches are given to for a HTTP / GRPC Route Rule, the controller doesn't generate any ALB rules. This behavior is incorrect as per the Gateway API documentation:

For GRPC routes -

	// If no matches are specified, the implementation MUST match every gRPC request.

For HTTP routes -

	// If no matches are specified, the default is a prefix
	// path match on "/", which has the effect of matching every
	// HTTP request.

This PR implements the logic as specified ^^^. I also added unit tests to validate the change worked. I was able to validate my GRPC set up now works with this change.

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
  • 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 added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 21, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Aug 21, 2025
HttpSpecificRulePrecedenceFactor: &HttpSpecificRulePrecedenceFactor{},
CommonRulePrecedence: common,
}
httpRoutes = append(httpRoutes, match)
Copy link
Collaborator

@shuqz shuqz Aug 21, 2025

Choose a reason for hiding this comment

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

i think getHttpMatchPrecedenceInfo(&httpMatch, &match) is missing when route does not match. it is ok for most of them (headers/query params) because if those does not get specified,they will have default value 0 or false. but at least for pathType, we want to treat them like prefix /*

func getHttpMatchPrecedenceInfo(httpMatch *v1.HTTPRouteMatch, matchPrecedence *RulePrecedence) {
	matchPrecedence.HttpSpecificRulePrecedenceFactor.PathType = getHttpRoutePathType(httpMatch.Path)
	// httpMatch.Path.Value won't be nil, default is /
	matchPrecedence.HttpSpecificRulePrecedenceFactor.PathLength = len(*httpMatch.Path.Value)
	matchPrecedence.HttpSpecificRulePrecedenceFactor.HasMethod = httpMatch.Method != nil
	matchPrecedence.HttpSpecificRulePrecedenceFactor.HeaderCount = len(httpMatch.Headers)
	matchPrecedence.HttpSpecificRulePrecedenceFactor.QueryParamCount = len(httpMatch.QueryParams)

}

@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2025
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2025
@zac-nixon zac-nixon merged commit 2bebac5 into kubernetes-sigs:main Aug 22, 2025
6 of 9 checks passed
zac-nixon added a commit to zac-nixon/aws-load-balancer-controller that referenced this pull request Aug 26, 2025
zac-nixon added a commit that referenced this pull request Aug 26, 2025
* fix doc statement

* Revert "fix http / grpc route rule generation (#4316)"

This reverts commit 2bebac5.

* re-add grpc routes
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. 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