added redirect tests for core redirect features

[LiorLieberman]

What type of PR is this?
/kind feature
/area conformance

What this PR does / why we need it:
Add tests for core redirect features

Which issue(s) this PR fixes:

Issue #1103

Does this PR introduce a user-facing change?:
NONE

[k8s-ci-robot]

Welcome @LiorLieberman!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @LiorLieberman. 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/test-infra repository.

[LiorLieberman]

This is a draft working implementation to check core features for redirect.
@robscott I am not a fan of my own implementation 😅 for reasons we discussed offline.
mainly around the fact that that we initilaize the http client inside CaptureRoundTrip() and custom roundtripper implementations would have to customize their logic in order to be able to test conformance for redirects.

As per #1496 I think that we should add a roundTripper under DefaultRoundTripper to implementations to override it.

Perhaps we should continue with this lean implementation and amend accordingly once #1496 is finished?

a few more nits;
I do not like adding RedirectRequest type and IsRedirect() func under the roundtripper package but I did have an import cycle so had to do it. strong thoughts on it ?

[robscott]

Thanks @LiorLieberman, this LGTM, just a few minor nits.

[robscott]

Even though we only allow 301 and 302, I'd make this logic more future-proof to include the full set of redirect codes. There's a chance we'll add more in the future, but even if not, it will be more helpful to show a more useful error message than if we didn't capture the redirect info.

[LiorLieberman]

Can you elaborate? my thought was to put it in a function so we will have a one central place for adding more codes in the future. I think now we want to fail conformance if someone returns something other than 301/302 because we do not support it, correct?

[robscott]

Let's say an implementation is actually returning a 307 or 308. With the current code, we wouldn't actually populate the rest of the redirect info for the rest of the test logic, so it would just look like everything failed instead of just having the wrong status code set.

I think the IsRedirect function is primarily a check on if we should populate the new redirect struct. The roundtripper itself should not care about what is conformant or not, just in making sure it populates as much information as possible so the tests can determine if the response was conformant.

[LiorLieberman]

Thats a good point, however we do check for wrong status code

gateway-api/conformance/utils/http/http.go

Lines 194 to 196 in c1c4952

	if expected.Response.StatusCode != cRes.StatusCode {
	return fmt.Errorf("expected status code to be %d, got %d", expected.Response.StatusCode, cRes.StatusCode)
	}

I agree the roundtripper should not care about conformance, would you prefer to construct the redirect struct always? Whats the value you think it'll give us? it wont change the behavior of the above lines - we will have a failure with a wrong status code still.

[robscott]

I think we should always construct the redirect struct for any 3xx status code. Although the test will still fail, it will likely provide more useful information to the person debugging the test. Instead of simply knowing the code is wrong, they'll also be able to see if the location they are redirecting to has any issues.

[LiorLieberman]

Makes sense, I have added all the codes but if you prefer I will do something like this instead let me know

func IsRedirect(statusCode int) bool {
	switch {
	case statusCode >= 300 && statusCode < 400:
		return true
	}
	return false
}

[robscott]

/ok-to-test

[shaneutt]

+1 to Rob's comments but they appear to be resolved already.

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman, shaneutt

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:

• conformance/OWNERS [shaneutt]

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

[robscott]

🤔 looks like @k8s-ci-robot got stuck on the LGTM, adding another.

/lgtm