Skip to content

[feat gw-api] backfill E2E tests #4264

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 16, 2025
Merged

[feat gw-api] backfill E2E tests #4264

merged 1 commit into from
Jul 16, 2025

Conversation

shuqz
Copy link
Collaborator

@shuqz shuqz commented Jul 14, 2025

Description

  • backfil E2E test for httproute match and filter feature, mostly added verifier for rule in E2E test
  • added some unit tests
  • added exception for other header modification since we only support redirect
  • added function for building target stickiness (this will be used later)
  • fixed some minor issues i saw during implementation

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
ginkgo -v -r test/e2e/gateway --  --kubeconfig=$KUBECONFIG --cluster-name=xxx --aws-region=xx --aws-vpc-id=vpc-xx --enable-gateway-tests -ginkgo.focus="test k8s alb gateway using instance targets reconciled by the aws load balancer controller|test k8s alb gateway using ip targets reconciled by the aws load balancer controller"

with result: 
------------------------------
SS

Ran 6 of 14 Specs in 1825.423 seconds
SUCCESS! -- 6 Passed | 0 Failed | 0 Pending | 8 Skipped
PASS
  • 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 14, 2025
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 14, 2025
shraddhabang
shraddhabang reviewed Jul 14, 2025
View reviewed changes
shraddhabang
shraddhabang reviewed Jul 14, 2025
View reviewed changes
shuqz
shuqz commented Jul 14, 2025
View reviewed changes
shuqz
shuqz commented Jul 14, 2025
View reviewed changes
shraddhabang
shraddhabang reviewed Jul 15, 2025
View reviewed changes
test/e2e/gateway/alb_instance_target_test.go Outdated
Protocol: gwv1.HTTPProtocolType,
},
}
httpr := buildHTTPRouteWithMatchesAndTargetGroupWeights()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of creating a dedicated func to build HTTP Routes with Matches and TG weights, Can you update the existing buildHTTPRoute function and send the required Filters and Matches for the rules. That way we can extend the same func to to process different testcases in future. I modified the same func to accept different hostnames we can extend it to accept filters and matches too.

Copy link
Collaborator Author

@shuqz shuqz Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

my intentional was to keep them independent since those will not be used in basic httproute tests. especially there are cases that when request redirect cannot be together with backendRefs, instead of handling all those in one function, i felt rather to keep them clean

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could customize the httpRoutes with our without backend refs too if needed later. I think rather than creating dedicated func for each of them it will be easier to keep one buildHTTPRoute func so that we can customize it as much as we want. Then we only need to think about the possible combinations of Filters, Matches to build test cases easily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i can make this change if it is preferred by you. i am fine with both

shraddhabang
shraddhabang reviewed Jul 15, 2025
View reviewed changes
@shuqz shuqz force-pushed the shuqz-httproute branch from c06d0be to f2c4495 Compare July 15, 2025 05:34
shraddhabang
shraddhabang reviewed Jul 15, 2025
View reviewed changes
shraddhabang
shraddhabang reviewed Jul 15, 2025
View reviewed changes
zac-nixon
zac-nixon reviewed Jul 15, 2025
View reviewed changes
zac-nixon
zac-nixon reviewed Jul 15, 2025
View reviewed changes
pkg/gateway/routeutils/route_rule_condition.go
@@ -98,7 +98,8 @@ func buildHttpHeaderCondition(headers []gwv1.HTTPHeaderMatch) []elbv2model.RuleC
Field: elbv2model.RuleConditionFieldHTTPHeader,
HTTPHeaderConfig: &elbv2model.HTTPHeaderConditionConfig{
HTTPHeaderName: string(header.Name),
Values: []string{header.Value},
// for a given HTTPHeaderName, ALB rule can accept a list of values. However, gateway route headers only accept one value per name, and name cannot duplicate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. Are we going to use the rule CRD to enhance this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, that's not discussed yet. i am ok to add it later when customer needs it.

Copy link
Collaborator Author

@shuqz shuqz Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

let me know if you want me to add it in rule-CRD, let's discuss it tomorrow

zac-nixon
zac-nixon reviewed Jul 15, 2025
View reviewed changes
@shuqz
[feat gw-api] backfill E2E tests
ff10dbb 
ignore makefile

address comments

refactor code
@shuqz shuqz force-pushed the shuqz-httproute branch from f2c4495 to ff10dbb Compare July 15, 2025 21:48
@shraddhabang
Copy link
Collaborator

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2025
@shraddhabang shraddhabang added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 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

@zac-nixon
Copy link
Collaborator

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot merged commit 3241ca9 into kubernetes-sigs:main Jul 16, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shraddhabang shraddhabang shraddhabang left review comments

@zac-nixon zac-nixon zac-nixon approved these changes

@wweiwei-li wweiwei-li Awaiting requested review from wweiwei-li

Assignees

@zac-nixon zac-nixon

@shraddhabang shraddhabang

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shuqz @shraddhabang @k8s-ci-robot @zac-nixon