Skip to content
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

e2e: tests for TCPRoute #2109

Merged
merged 5 commits into from
Nov 17, 2023
Merged

e2e: tests for TCPRoute #2109

merged 5 commits into from
Nov 17, 2023

Conversation

slayer321
Copy link
Contributor

What type of PR is this?

e2e: tests for TCPRoute

What this PR does / why we need it:
Add the e2e test for TCPRoute
Which issue(s) this PR fixes:

Fixes #2010

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (768e4a2) 64.19% compared to head (98ab0ab) 64.22%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2109      +/-   ##
==========================================
+ Coverage   64.19%   64.22%   +0.03%     
==========================================
  Files         107      107              
  Lines       14924    14924              
==========================================
+ Hits         9580     9585       +5     
+ Misses       4768     4764       -4     
+ Partials      576      575       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}

// Send a request to an valid path and expect a successful response
http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, OkResp)
Copy link
Contributor

Choose a reason for hiding this comment

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

how is the destination port (8090, 8080) being computed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand following the docs. We have one Gateway resource(and two TCP listerns on port 8080 and 8090) and two TCPRoute resources. When e2e test is run gateways are created and requests are passed to the added services port for TCPRoute. (Please correct me if I'm missing something here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While checking for WaitForGatewayAddress function found that it only get's the port of first listeners . In our case we have two listeners sets on (8080, 8090) , but as It is just getting the first port of the listener i.e 8080 . After running the e2e test I found that it is making request on same port for both listeners

tcp-route.go:83: Making GET request to http://172.18.255.205:8080/
tcp-route.go:102: Making GET request to http://172.18.255.205:8080/

Copy link
Contributor

Choose a reason for hiding this comment

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

can you ensure traffic is sent to different ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Now traffic is send to specific ports

tcp-route.go:78: Making GET request to http://172.18.255.205:8090/
tcp-route.go:60: Making GET request to http://172.18.255.205:8080/

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321 slayer321 marked this pull request as ready for review November 16, 2023 11:46
@slayer321 slayer321 requested a review from a team as a code owner November 16, 2023 11:46

// WaitForGatewayAddress waits until at least one IP Address has been set in the
// status of the specified Gateway.
func WaitForGatewayAddress(t *testing.T, client client.Client, timeoutConfig config.TimeoutConfig, gwName types.NamespacedName, sectionName string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For TCP test we cannot use it as we are using two listeners here and as I mentioned in this comment inside the WaitForGatewayAddress function they are only checking the port for the first listeners. Here I have update the WaitForGatewayAddress function to take the sectionName and select the port based on that and if the sectionName is empty it will bydefault take the first listeners port.

can the other helper functions be reused from upstream ?

Yes, I'm mostly using all the helper function from upstream and as we already have the udp test merged I'm using some of the upstream code from there as there is no need to duplicate it .

Xunzhuo and others added 2 commits November 17, 2023 16:17
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks

Copy link
Contributor

@zirain zirain left a comment

Choose a reason for hiding this comment

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

Thanks, hope we can move these to upstream one day.

@zirain zirain merged commit 7f36021 into envoyproxy:main Nov 17, 2023
18 checks passed
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.

Add E2E tests for TCPRoute
4 participants